Conversation
jlesquembre
left a comment
jlesquembre
left a comment
There was a problem hiding this comment.
Great to see the labeler work as expected :)
It looks good to me.
Just a minor thing, now that we have the .clang-format file in place, would you like to consider applying it to future changes for consistency?
|
Right! I didn't connect the dots. Will do now as well. |
Thunks are relevant when initializing attrsets and lists, passing arguments. This is an important way to produce them.
roberth
force-pushed
the
c-api-nix-value-init-apply
branch
from
b8f3b87 to
ad643cd
Compare
|
Thanks for the review. I'll also revive my pre-commit hooks PR now that we have more files we can apply that to. |
| * @see nix_value_call() for a similar function that performs the call immediately and only stores the return value. | ||
| * Note the different argument order. | ||
| */ | ||
| nix_err nix_init_apply(nix_c_context * context, Value * value, Value * fn, Value * arg); |
There was a problem hiding this comment.
Not having an EvalState here restricts the implementation of the C API to implementations of Nix that have a global garbage collector or equivalent; there's no reason to not take an EvalState that isn't used in the API for future proofing.
And, ignoring that, having the distinct argument ordering feels like it's a bit of a footgun.
There was a problem hiding this comment.
Both issues follow from the format of the other initializers, but I'm not sure that it's a problem in this case, as the implementation doesn't allocate. Nonetheless, it's a risk, I suppose.
To be on the safe side, we could
- Change the argument order to put the out parameter last
- Add an
EvalState *, which will be unused - s/nix_init/nix_value_init? This is extra breaking, but that's actually helpful I think. The API is still in development, so that's ok.
EDIT: added issue #10577
3 participants
Motivation
Thunks are relevant when initializing attrsets and lists, or passing arguments to functions that are lazy in those arguments. This is an important way to produce them.
Context
cc @jlesquembre
Also, the labeler worked :)
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.