-
Notifications
You must be signed in to change notification settings - Fork 146
Allowing constants in the expressions of derived variables #4644
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
c60b9a4 to
75af359
Compare
|
This is ready for review, I cleaned the history so it's easier to follow all the changes. The testing passed when the derived variables were on but I turned them off for merging. |
eisenhauer
left a comment
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.
Worried that you've got ambiguity here.
How does "X-2" tokenize and parse? Same as "x -2" and "x - 2"? Or do the first two of those come through as VAR NUM rather than VAR OP NUM? Because both of those would be valid with these definitions where the - might be a separate operator or it might be part of the constant.
The C and C++ specs don't associate the '-' with the constant. Instead if you have a negative constant it goes through lexical analysis separately and parses as a unary operator on the positive constant. That approach produces an unambiguous result for all of those inputs. Now, we don't currently have a unary operator in the language, which is probably also an issue, because negation isn't exactly unheard of.
|
For this PR I picked parts of what Liz did to capture numbers. The parser line is: so I do agree that d-2 could potentially be VAR NUM or VAR OP NUM When I try "d-2" this is tokenized as VAR OP NUM on my laptop (don't ask me why) which is fine if we supported constants for minus (we don't but should be easy to add if we want to). But the problem you are showing is bigger than that. Surprisingly X * -2 or -2 * X all give errors during parsing and will not detect NUM. |
|
The testing passes when the derived variables are on. Frontier fails because of an unrelated test: As far as I can tell this test doesn't have any derived variables. |
689ce06 to
7013d1e
Compare
…class Co-authored-by: Liz Dulac <[email protected]>
7013d1e to
3b01c00
Compare
|
I had to turn derived variables off, @eisenhauer please approve again |
|
At some point are we going to leave these on? |
|
It should be done in one of the releases (as soon as we consider that the API will not change anymore), we can discuss if we should turn them on by default in the next release or not. Frontier is still failing (unrelated to this PR) and I still don't have permission to overwrite this. If you force merge @eisenhauer keep the commits (the history is clean). This way I can check the one that removed the operators when Scott will want them back. |
|
Hmm. Frontier is not in the required set, so if you have write permissions, you should be able to merge yourself (I see the option without the need to override). But I can do it if there is some issue. But you don't want to Squash? Just a simple merge commit? |
|
Ah you are right, Frontier is not :) Yes, I didn't want to squash |
This PR does the following:
These are supported now:
Not supported (until we have a better grammar):