Skip to content

Expose yyatEOF() in generated scanner API#644

Merged
regisd merged 14 commits intojflex-de:masterfrom
regisd:zzateof
Dec 3, 2019
Merged

Expose yyatEOF() in generated scanner API#644
regisd merged 14 commits intojflex-de:masterfrom
regisd:zzateof

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 Feature requests 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 December 1, 2019 12:02
@regisd regisd self-assigned this Dec 1, 2019
@regisd
Copy link
Member Author

regisd commented Dec 1, 2019 •
edited
Loading

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 •
edited
Loading

@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
View reviewed changes
Copy link
Member

@lsf37 lsf37 left a comment •
edited by regisd
Loading

Choose a reason for hiding this comment

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

[deleted]

@lsf37 lsf37 self-requested a review December 1, 2019 23:08
lsf37
lsf37 requested changes Dec 1, 2019
View reviewed changes
Copy link
Member

@lsf37 lsf37 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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 •
edited
Loading

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 December 3, 2019 00:45
lsf37
lsf37 approved these changes Dec 3, 2019
View reviewed changes
Copy link
Member

@lsf37 lsf37 left a comment

Choose a reason for hiding this comment

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

Perfect. Good to merge!

@regisd regisd merged commit f966df4 into jflex-de:master Dec 3, 2019
@regisd regisd deleted the zzateof branch December 3, 2019 13:12
regisd pushed a commit that referenced this pull request Dec 3, 2019
@traviscibot
Pseudo-Merge f966df4 Expose yyatEOF() in generated scanner API (#644)
5796701 
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

Reviewers

@lsf37 lsf37 lsf37 approved these changes

Assignees

@regisd regisd

@lsf37 lsf37

Labels

enhancement Feature requests

Projects

None yet

Milestone

1.8.0

Development

Successfully merging this pull request may close these issues.

2 participants

@regisd @lsf37