@Ericson2314 Now that I'm making this PR I realise we might have it backwards. most of the init functions don't need to allocate anything on the heap. nix_alloc_value does that, and it already has an EvalState argument

Only the string and path init functions allocate StringData on the heap.

I suppose separate vectors per type will also break that? (Though it is also less clear we can/should do that for values than exprs.)

Yeah that design is (a) not necessary for Values (almost all of them will fit naturally in 8 bytes) (b) not possible for Values (they start out as thunks and then become another type when they're forced)

Though after sleeping on it, I realise that we will need an EvalState (or at least EvalMemory) just to access the Values, not only to allocate them.

Though I also realised there may be a way to shoehorn all of this into the old API.

Are we treating types like nix_value as opaque to the API? Because in that case we could stuff an EvalState * (or EvalMemory *) into nix_value.

Side note: eventually nix_value will need to change (again). If we're referencing Values by index, we can't let the user of the API decide where they're stored, They'll have to work with ValueRefs

(It would be possible to make forced thinks a double indirection, which is then cleaned up at GC time, but yes I am suspicious about that.)

I lean towards:

• go straight to ValueRef
• no more allocating a value and then deciding what to put in it, have to do both at once (but of course making sure a thunk defers the choice of what it will eventually be)
• ValueRef should not be double wide, need to access with EvalState

I think those choices make for something which is compatible with many types of heap designs.

@Radvendii the types are intentionally opaque so that their implementation contents can be adjusted as needed.

Changing these signatures should really be a last resort. Storing something like EvalState * may not be pretty, but gets the job done, and that's the purpose of this layer; make it work without imposing caller side changes.
Something along the lines of nix_value = std::variant<Value *, std::function<Value *(EvalState *)>> would also get the job done, but storing the EvalState * is simpler in terms of implementation.

What would be the purpose of the std::variant?

Also want to note that changing the size of a type is an observable effect on consumers of the API, even if the type is opaque. Maybe we're expecting people to not depend on that, I just wanted to bring it up in case it needs to be considered.

nix_value being {EvalState *, Value *} does work.

Oh @Ericson2314 good point that forced thunks could be an indirection. I don't think there's a compelling reason to design it this way, but I haven't given it much thought yet. I'll bear this in mind.

I just realised that nix_value is always passed around the API as nix_value *, which means
(a) it was already not the case that the user of the API could store values wherever they want
(b) the user of the API doesn't care how big nix_value is.
(c) we have to allocate the nix_value on the GC heap and refcount it instead of the value, i guess?

@Ericson2314 I think this means that we don't need to worry about changing Value to Value * right now. That's totally opaque to the user of the API anyways.

What would be the purpose of the std::variant?

Delayed construction when the necessary EvalState * is available. (Sorry; I was entirely unclear)

I do like that it doesn't store the EvalState * in nix_value, which is somewhat nicer in terms of data model, but probably not worth the hassle of doing delayed construction.

We can make a new v2 API after we are done, and hack as needed to make the old one back-compat.

@Radvendii good to close this then, I think?

Yep! Closing as changing the c api is not necessary. I'll update #14584 so it changes nix_value instead.

I'm a bit late to the party, but I'd like to note that since usually operator new hands out (on 64 bit system) already 16 byte aligned pointers we are not wasting any more memory by keeping an EvalState * in nix_value.

nix_value being {EvalState *, Value *} does work.

Hmm... Thinking about this more I don't know what you mean @xokdvium.

nix_value is currently essentially a type alias for Value (16 bytes).

Changing it to {EvalState *, Value} makes it 24 bytes (32 because of alignment apparently).

Changing it to {EvalState *, Value *} Uses 16 bytes, and then an additional 16 bytes for the Value itself.

Either way we double the memory footprint of nix_value.

You can only have a reference to the value state (i.e. Value, not sure if you're repurposing that name) in nix_value, because if you duplicate the value state, you're also duplicating the computational work and other side effects if any.

nix_value itself has a nix::Value embedded directly (I am not repurposing the name). The thing passed around is a nix_value * which corresponds to a Value *.

The code uses reinterpret_cast to get between nix_value * and Value *

I think maybe this design was a minor mistake, and it would have made more sense to make nix_value contain a Value * and pass around nix_values, but that's not how the code works currently, and I might be missing some drawback to that design.

Ohhh wait if you're talking about the changes I'm making now, I was coming to the same conclusion you were. It can't be {EvalState *, Value}, it must be {EvalState *, Value *}