lix-project/lix

Fork 48

remove `-Wno-deprecated-declarations` #744

New issue

Open

opened 2025-03-18 09:17:09 +00:00 by pennae · 6 comments

pennae commented

2025-03-18 09:17:09 +00:00

Owner

we currently build lix with -Wno-deprecated-declarations, but we'd like to turn that warning back on. right now this isn't feasible because we have a bunch of these warnings caused by our own declarations (which are rather easy to fix) and declarations from openssl, rapidcheck, and some other places. this should be easy to do, but there are many places that cause these warnings

we currently build lix with `-Wno-deprecated-declarations`, but we'd like to turn that warning back on. right now this isn't feasible because we have a bunch of these warnings caused by our own declarations (which are rather easy to fix) and declarations from openssl, rapidcheck, and some other places. this should be easy to do, but there are many places that cause these warnings

pennae added the

devx

E/easy

E/help wanted

labels

2025-03-18 09:17:09 +00:00

lix-bot commented

2025-03-18 09:18:05 +00:00

Member

This issue was mentioned on Gerrit on the following CLs:

comment in cl/2829 ("n-e-j: use full lix warning config")
commit message in cl/2934 ("libutil: use openssl EVP for hashing, address deprecated use")
commit message in cl/2969 ("tests: ignore deprecated uses in rapidcheck")

This issue was mentioned on Gerrit on the following CLs: * comment in [cl/2829](https://gerrit.lix.systems/c/lix/+/2829) ("n-e-j: use full lix warning config") * commit message in [cl/2934](https://gerrit.lix.systems/c/lix/+/2934) ("libutil: use openssl EVP for hashing, address deprecated use") * commit message in [cl/2969](https://gerrit.lix.systems/c/lix/+/2969) ("tests: ignore deprecated uses in rapidcheck")

fireyfly commented

2025-04-05 11:18:06 +00:00

Member

I was looking into this a bit.. the warnings within our code mainly stem from use of the default Value constructor. There are many easy cases (esp in tests etc) but the main tricky case I noticed was the prevalence of eval-y functions (EvalState::eval, NixRepl::evalString, EvalState::callFunction etc) writing the result to an out parameter passed by reference rather than returning it (by value).

This means there's lots of calls like Value v; state.evalFile(CanonPath(manifestFile), v); or Value v2; state.autoCallFunction(*autoArgs, v1, v2);. I'm not super clear on what the desired factoring of this would be.. all (or almost all?) of these functions seem to be void-return functions currently, so one option might be to refactor them to return their result by value instead of with an out-parameter, but I'm not sure if there's performance reasons for avoiding that?

I kinda already went ahead and tried that locally (just out of curiosity) with the functions where it's relevant for purposes of avoiding Value default constructors, and at least tests pass locally + some quick smoketests seem to work...

I was looking into this a bit.. the warnings within our code mainly stem from use of the default `Value` constructor. There are many easy cases (esp in tests etc) but the main tricky case I noticed was the prevalence of eval-y functions (`EvalState::eval`, `NixRepl::evalString`, `EvalState::callFunction` etc) writing the result to an out parameter passed by reference rather than returning it (by value). This means there's lots of calls like `Value v; state.evalFile(CanonPath(manifestFile), v);` or `Value v2; state.autoCallFunction(*autoArgs, v1, v2);`. I'm not super clear on what the desired factoring of this would be.. all (or almost all?) of these functions seem to be `void`-return functions currently, so one option might be to refactor them to return their result by value instead of with an out-parameter, but I'm not sure if there's performance reasons for avoiding that? I kinda already went ahead and tried that locally (just out of curiosity) with the functions where it's relevant for purposes of avoiding `Value` default constructors, and at least tests pass locally + some quick smoketests seem to work...

pennae commented

2025-04-05 11:35:50 +00:00

Author

Owner

having the Value-out-parameter functions return a value has indeed been tried in the past, but for reasons that were never investigated deeply we saw a noticable negative perf impact for that (cl/1626). having them return values would be awesome, but we really have to get the perf impact under control

having the Value-out-parameter functions return a value has indeed been tried in the past, but for reasons that were never investigated deeply we saw a noticable negative perf impact for that (cl/1626). having them return values would be awesome, but we really have to get the perf impact under control

👍 1

kfearsoff commented

2025-04-06 23:14:03 +00:00

Member

Wow, that's a bizarre perf regression. Was this on Clang, gcc or both?

pennae commented

2025-04-07 10:02:40 +00:00

Author

Owner

don't remember. this was tested before we forbade building with gcc, so chances are high it happened on gcc

fireyfly commented

2025-04-07 20:01:45 +00:00

Member

I took a stab at the perf regressions and took the liberty of forking it out into its own Forgejo issue: #792

I took a stab at the perf regressions and took the liberty of forking it out into its own Forgejo issue: https://git.lix.systems/lix-project/lix/issues/792