lix-project/lix

Fork 65

remove `-Wno-deprecated-declarations` #744

New issue

Open

opened 2025-03-18 09:17:09 +00:00 by pennae · 7 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")
comment in cl/5219 ("libexpr: migrate Expr::eval to return a Value, take 2")
commit message in cl/5305 ("libexpr: Migrate EvalState::mkOutputString to return a Value")
commit message in cl/5306 ("libexpr: Replace Value::mkNull with Value::VNULL")
commit message in cl/5304 ("libexpr: replace BindingsBuilder::alloc with insert")
commit message in cl/5301 ("libexpr: Migrate makePositionThunks to return Value tuple")
commit message in cl/5308 ("libexpr: Migrate EvalState::mkPos to return a Value")
commit message in cl/5309 ("libexpr: Migrate emitTreeAttrs to return a Value")
commit message in cl/5345 ("libexpr/attr-set.hh: Deprecate Attr default constructor")
commit message in cl/5344 ("libexpr: Push Values onto vectors instead of default constructing ahead of time")
commit message in cl/5349 ("libexpr: Remove various default constructions of Values")
commit message in cl/5350 ("libexpr/nixexpr: Move backing fields of ExprLiteral subclasses into new base classes")
commit message in cl/5352 ("libexpr/primops: Avoid Value default construction in primop_removeAttrs")
commit message in cl/5357 ("meson.build: Warn on deprecated declarations")
commit message in cl/5356 ("libexpr/eval-expr: extract makeThunk function that constructs and increments counters")
commit message in cl/5355 ("(WIP) libexpr/json-to-value: Rewrite json-to-value.cc")

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") * comment in [cl/5219](https://gerrit.lix.systems/c/lix/+/5219) ("libexpr: migrate Expr::eval to return a Value, take 2") * commit message in [cl/5305](https://gerrit.lix.systems/c/lix/+/5305) ("libexpr: Migrate EvalState::mkOutputString to return a Value") * commit message in [cl/5306](https://gerrit.lix.systems/c/lix/+/5306) ("libexpr: Replace Value::mkNull with Value::VNULL") * commit message in [cl/5304](https://gerrit.lix.systems/c/lix/+/5304) ("libexpr: replace `BindingsBuilder::alloc` with `insert`") * commit message in [cl/5301](https://gerrit.lix.systems/c/lix/+/5301) ("libexpr: Migrate makePositionThunks to return Value tuple") * commit message in [cl/5308](https://gerrit.lix.systems/c/lix/+/5308) ("libexpr: Migrate EvalState::mkPos to return a Value") * commit message in [cl/5309](https://gerrit.lix.systems/c/lix/+/5309) ("libexpr: Migrate emitTreeAttrs to return a Value") * commit message in [cl/5345](https://gerrit.lix.systems/c/lix/+/5345) ("libexpr/attr-set.hh: Deprecate Attr default constructor") * commit message in [cl/5344](https://gerrit.lix.systems/c/lix/+/5344) ("libexpr: Push Values onto vectors instead of default constructing ahead of time") * commit message in [cl/5349](https://gerrit.lix.systems/c/lix/+/5349) ("libexpr: Remove various default constructions of `Value`s") * commit message in [cl/5350](https://gerrit.lix.systems/c/lix/+/5350) ("libexpr/nixexpr: Move backing fields of ExprLiteral subclasses into new base classes") * commit message in [cl/5352](https://gerrit.lix.systems/c/lix/+/5352) ("libexpr/primops: Avoid Value default construction in primop_removeAttrs") * commit message in [cl/5357](https://gerrit.lix.systems/c/lix/+/5357) ("meson.build: Warn on deprecated declarations") * commit message in [cl/5356](https://gerrit.lix.systems/c/lix/+/5356) ("libexpr/eval-expr: extract makeThunk function that constructs and increments counters") * commit message in [cl/5355](https://gerrit.lix.systems/c/lix/+/5355) ("(WIP) libexpr/json-to-value: Rewrite json-to-value.cc")

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