Skip to content

Resolve ANTLR warnings by defining named lexer tokens #225

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

Merged
merged 4 commits into from
Apr 8, 2021

Conversation

RexJaeschke
Copy link
Contributor

@RexJaeschke RexJaeschke commented Feb 17, 2021

This PR addresses Issue #37, Proposal 4 (#37 (comment)). Specifically, it

  • Adds definitions for 6 named lexer tokens, and then refers to them in various lexer rule alternatives
  • Adds some ANTLR-related commentary
  • Defines New_Line as a strict superset of New_Line_Character
  • Fixes an error in Single_Regular_String_Literal_Character
  • Changes identfier => Identifier in expressions.md
  • Changes Right_shift => Right_Shift in classes.md

For now this is a Draft PR, as I want to explore the idea of replacing the use of certain terminals in the syntactic grammar with the 6 new named lexer tokens (for example, 'default' in a switch statement becomes DEFAULT).

I explored that and decided to not go down that path, as explained in #37 (comment).

@RexJaeschke RexJaeschke marked this pull request as ready for review February 17, 2021 21:21
@RexJaeschke RexJaeschke added this to the C# 6 milestone Feb 17, 2021
@jskeet jskeet added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Mar 5, 2021
SLASH : '/' ;
```

Although these are lexer rules, these names are spelled in all-uppercase letters to distinguish them from ordinary lexer rule names. These rules are also written on a single line.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "These rules are also written on a single line." is redundant and better omitted.

Copy link
Contributor

@Nigel-Ecma Nigel-Ecma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat ambivalent on the calling out of a few tokens as lexical rules and the use of uppercase for them, I'm not sure it enhances the grammar or that it harms it either; however the changes seem correct and I'm therefore happy to approve. I've suggested one line is redundant and can therefore be removed.

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Nigel's comment about the extraneous line.

I believe there was a good reason for the all-caps lexer tokens, although I can't now remember what it was :)

@jskeet
Copy link
Contributor

jskeet commented Apr 7, 2021

Note for meeting: I don't think there have been changes since the last meeting, so we may wish to skip this.

@RexJaeschke RexJaeschke removed the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Apr 7, 2021
@RexJaeschke RexJaeschke merged commit d60381f into draft-v6 Apr 8, 2021
@RexJaeschke RexJaeschke deleted the Issue-37-Proposal-4 branch April 8, 2021 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants