-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
parser: Remove empty multiline string parts earlier #11120
Conversation
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.
LGTM
Hold on, I think I found a minor problem with this, will follow-up |
So that in the next commit we can see what changes about this test
0093bf1
to
23c44d9
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.
Alright should be good now.
src/libexpr/parser-state.hh
Outdated
/* If there is nothing at all, return the empty string directly */ | ||
if (es2->size() == 0) { | ||
auto *const result = new ExprString(""); | ||
delete es2; | ||
return result; | ||
} | ||
|
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.
The problem I discovered is fixed by this. This is necessary because newly es2
can be entirely empty, in which case we should not create an ExprConcatStrings
, but just return an empty string directly. Not doing so would give inconsistent results from
''''
(which would newly create an ExprConcatStrings
, allocating a thunk)
and
''
''
(which would be turned into ""
)
This is now covered in the test.
Makes parsing more consistent and is a super minor optimisation Co-authored-by: Robert Hensing <[email protected]>
20e916b
to
0c91bb9
Compare
Applied and rebased with your suggestion :) |
Motivation
Consider these two expressions:
They parse differently:
The first one also ends up allocating an extra thunk to be evaluated, but they evaluate to the same:
There is no need for this inconsistency, so this PR changes it to treat both the same (
a.nix
now acts the same asb.nix
). This is done by preventing the propagation of empty strings to be concatenated in the parser already.Context
I've found this when I tried to verify that
nix-instantiate --parse
doesn't change upon applying a formatter on Nixpkgs. Turns out it does change at least because of this.Priorities and Process
This work is sponsored by Antithesis ✨
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.