-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
libexpr: store ExprLambda data in Expr::alloc #14384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
80068f9 to
5db63f3
Compare
5c5b4ea to
06be0b0
Compare
e.g. (foo@{}: 1) { a = 3; } should error, but wasn't with the previous
commit
7e60c5a to
67be2df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the way it looks now, thank you very much @Radvendii!
| } | ||
| std::ranges::copy(formals.formals, formalsStart); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a check in case there a more than 65k formals. We must fail gracefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would overflow the heap buffer otherwise. Let's please not do this.
| , formals(formals) | ||
| , hasFormals(true) | ||
| , ellipsis(formals.ellipsis) | ||
| , nFormals(formals.formals.size()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can do a silent truncation from size_t -> uint16_t.
Motivation
For big-picture motivation, see the tracking issue
This PR moves the ExprLambda data into our allocator
{ foo, bar }:rather thanx:)Formals *formalsFormal *And everything else stays the same.
Alternative Design
Presumably we were using aOne effect of usingFormals *here rather thanFormalsdirectly so that we don't need to take up the extra space in the common case that there are no formals.Formals *rather thanFormalsdirectly has been that we don't need to take up the extra space to store aFormalsin the common case that there are no formals.This PR does not make that case any worse, because we can fit the extra data (ellipses + hasFormals + nFormals) in the space that was padding before.
However, we could also choose to put all formals in one array and index into it, reducing the 8 byte
Formal *to a 4 byte index.IMO this is an optimization to look into in the future, when we start putting things in indexable vectors. (e.g.
Exprs)Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.