Skip to content
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

Expose yyatEOF() in generated scanner API #644

Merged
merged 14 commits into from Dec 3, 2019
Merged

Conversation

regisd
Copy link
Member

@regisd regisd commented Dec 1, 2019

I don't think there is a convenient way to known whether a scanner has finished reading its input.

But it's sounds to me that looping until EOF is a critical use case, which is even implemented in the standalone main method as:

while ( !scanner.zzAtEOF ) scanner.yylex();

See Emitter.java:350

This change adds a getter for the zzAtEOF field.

@regisd regisd added the enhancement label Dec 1, 2019
@regisd regisd added this to the 1.8.0 milestone Dec 1, 2019
@regisd regisd requested a review from lsf37 as a code owner Dec 1, 2019
@regisd regisd self-assigned this Dec 1, 2019
@regisd regisd added this to Incoming Reviews in JFlex core via automation Dec 1, 2019
@regisd
Copy link
Member Author

regisd commented Dec 1, 2019

The benefit is quite clear on EofminGoldenTest.java which has states and doesn't have a single token for the EOF status.

@regisd
Copy link
Member Author

regisd commented Dec 1, 2019

@lsf37 Am I missing something? Can a client of the scanner know whether the scanner has reached EOF in a generic way?

@regisd
Copy link
Member Author

regisd commented Dec 1, 2019

@lsf37 I chose name zzAtEof() to be consistent with the field name, but why a zz prefix? It could also be yyAtEof() or just isAtEof(). What do you prefer?

@lsf37
Copy link
Member

lsf37 commented Dec 1, 2019

The zz prefix means that something is internal, i.e. not part of the API. If we make it a getter method that we want people to use, it should therefore be yyAtEOF(). We could also rename the field, but it makes sense to leave that internal, because we might change the mechanism by which we figure out the end of file.

I don't fully remember any more wether relying on zzAtEOF is always safe. Will investigate.

The whole yy and zz naming scheme is from the JLex days (at least yy is, and then I think I used zz to distinguish). The idea was to avoid conflicts with names that users might want to pick. For the actually internal parts that makes sense, but the API would probably look less strange if it didn't have a prefix. In any case, for new things we should stay consistent.

lsf37
lsf37 approved these changes Dec 1, 2019
Copy link
Member

@lsf37 lsf37 left a comment

[deleted]

JFlex core automation moved this from Incoming Reviews to In progress Dec 1, 2019
@lsf37 lsf37 self-requested a review Dec 1, 2019
Copy link
Member

@lsf37 lsf37 left a comment

Oops, last comment was for the wrong thread. I'm still investigating, but it looks like it's fine so far. Would only change zzAtEof() to yyAtEOF().

@lsf37
Copy link
Member

lsf37 commented Dec 1, 2019

Ok, after looking through the emitter and skeleton code, yyAtEOF() should be fine to export as API. I have vague memories of not doing that initially, because usually the scanner should return a token to indicate that it is done instead of the caller looking at scanner state, but that is really more a matter of taste, so no reason not to export. (Debug and standalone scanners used zzAtEOF because it was hard to figure out what the spec uses as EOF token).

@lsf37
Copy link
Member

lsf37 commented Dec 1, 2019

We should probably add it to the manual as well. I can do that if you like (happy for you to merge first).

@regisd
Copy link
Member Author

regisd commented Dec 2, 2019

Oops, last comment was for the wrong thread. I'm still investigating, but it looks like it's fine so far. Would only change zzAtEof() to yyAtEOF().

Renamed to yyatEOF() (lower case 'a' to be consistent with the rest) in commit ba9a74c.

@regisd regisd changed the title Expose zzAtEof() in generated scanner API Expose yyatEOF() in generated scanner API Dec 2, 2019
@regisd
Copy link
Member Author

regisd commented Dec 2, 2019

We should probably add it to the manual as well. I can do that if you like (happy for you to merge first).

Documentation should be added with code. Done in 4405977.

@regisd regisd requested a review from lsf37 Dec 3, 2019
lsf37
lsf37 approved these changes Dec 3, 2019
Copy link
Member

@lsf37 lsf37 left a comment

Perfect. Good to merge!

@regisd regisd merged commit f966df4 into jflex-de:master Dec 3, 2019
5 checks passed
JFlex core automation moved this from In progress to Done Dec 3, 2019
@regisd regisd deleted the zzateof branch Dec 3, 2019
regisd pushed a commit that referenced this issue Dec 3, 2019
commit f966df4
Author:     Régis Décamps <regisd@google.com>
AuthorDate: Tue Dec 3 14:12:41 2019 +0100
Commit:     GitHub <noreply@github.com>
CommitDate: Tue Dec 3 14:12:41 2019 +0100

    Expose `yyatEOF()` in generated scanner API (#644)

    * Add new `zzatEOF()` method to generated scanners.

    * Demonstrate use in tests

    * Update vm template in migration tool

    * Add documentation about yyatEOF()

Updated from target/jflex-parent-1.8.0-SNAPSHOT-sources.jar
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement
Projects
JFlex core
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants