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

VariableExpression: whitespace/hyphen/binop parsing fix #13134

Merged
merged 4 commits into from Apr 22, 2017

Conversation

Projects
None yet
4 participants
@atlimit8
Member

atlimit8 commented Apr 15, 2017

Fixed line 325 of OpenRA.Game/Support/ConditionExpression.cs as mentioned in #13130. I had decided that it would be easier to use spaces to disambiguate minus and negative since condition names could have hyphens, and then forgot to mention that. Binary arithmetic and logic operators require whitespace around them and CharClass.Mixed characters, including -, are restricted to the middle of variable names.

Closes #13130.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Apr 16, 2017

Member

@pchote, your t- 1 case made me reconsider lexing of trailing CharClass.Mixed characters and the hyphen in particular.

CharClass.Mixed characters are now restricted to the middle of identifiers and a - at the end is lexed into TokenType.Subtract.

Now t -1, t -1, t - 1, and t - 1 have the same AST and thus evaluation.

Member

atlimit8 commented Apr 16, 2017

@pchote, your t- 1 case made me reconsider lexing of trailing CharClass.Mixed characters and the hyphen in particular.

CharClass.Mixed characters are now restricted to the middle of identifiers and a - at the end is lexed into TokenType.Subtract.

Now t -1, t -1, t - 1, and t - 1 have the same AST and thus evaluation.

@atlimit8 atlimit8 changed the title from ConditionExpression minus/negative/no whitespace lexing fix to ConditionExpression: hyphen without whitespace lexing fix Apr 16, 2017

@pchote

The following testcase still fails:

AssertValue("one-33-five-10", -47); // returns 0 instead

This kind of expression is unfortunately used through most of the UI definitions.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 16, 2017

Member

It would make life easier if we disallowed - and other ambiguous characters in variable names. IMO we should consider doing this.

Member

pchote commented Apr 16, 2017

It would make life easier if we disallowed - and other ambiguous characters in variable names. IMO we should consider doing this.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 17, 2017

Member

We discussed this in IRC and decided that it would be better to keep support for hyphens (except as the last character) in variable names, and then explicitly require whitespace around all binary operations.

This will require some changes to the lexer and testcases, plus a lot of grunt work when porting the UI to the new parser, but the effort should pay off in cleaner logic and cleaner yaml.

Member

pchote commented Apr 17, 2017

We discussed this in IRC and decided that it would be better to keep support for hyphens (except as the last character) in variable names, and then explicitly require whitespace around all binary operations.

This will require some changes to the lexer and testcases, plus a lot of grunt work when porting the UI to the new parser, but the effort should pay off in cleaner logic and cleaner yaml.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Apr 17, 2017

Contributor

explicitly require whitespace around all binary operations.

Wait, even a whitespace after the ! operation? Others I agree with and support fully, but I'd say that one shouldn't be enforced.

Contributor

GraionDilach commented Apr 17, 2017

explicitly require whitespace around all binary operations.

Wait, even a whitespace after the ! operation? Others I agree with and support fully, but I'd say that one shouldn't be enforced.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 17, 2017

Member

! is a unary operator, and wouldn't need a space. In fact i'd argue that having a space there should be a syntax error.

Member

pchote commented Apr 17, 2017

! is a unary operator, and wouldn't need a space. In fact i'd argue that having a space there should be a syntax error.

@atlimit8 atlimit8 changed the title from ConditionExpression: hyphen without whitespace lexing fix to VariableExpression: whitespace/hyphen/binop lexing fix Apr 18, 2017

Reversal of requirements: minus operator must be surrounded by whitespace now.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Apr 18, 2017

Member

Applied new requirements as we agreed in the IRC channel.

Member

atlimit8 commented Apr 18, 2017

Applied new requirements as we agreed in the IRC channel.

@pchote

pchote approved these changes Apr 22, 2017

Looks reasonable 👍. Just one minor request:

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Apr 22, 2017

Member

Replaced comment - fixed.

Member

atlimit8 commented Apr 22, 2017

Replaced comment - fixed.

@atlimit8 atlimit8 changed the title from VariableExpression: whitespace/hyphen/binop lexing fix to VariableExpression: whitespace/hyphen/binop parsing fix Apr 22, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Apr 22, 2017

Contributor

Looking good, as far as I can tell. If any edge cases exist that were overlooked (I doubt it, but my own ability to understand code like this is still too shaky to be 100% certain), we can still fix them if we run into them.
👍

Contributor

reaperrr commented Apr 22, 2017

Looking good, as far as I can tell. If any edge cases exist that were overlooked (I doubt it, but my own ability to understand code like this is still too shaky to be 100% certain), we can still fix them if we run into them.
👍

@reaperrr reaperrr merged commit ac1d452 into OpenRA:bleed Apr 22, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr
Contributor

reaperrr commented Apr 22, 2017

@atlimit8 atlimit8 deleted the atlimit8:NegMinusExpressionParsing branch May 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment