HQLPARSER-39 Detection of unconsumed tokens must take non-buffered tokens into account #21

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Member

anistor commented Feb 26, 2014

  • ensure the internal token buffer of CommonTokenStream is filled from the underlying TokenSource (lexer) before checking if any tokens are left unmatched
  • detect EOF by comparing on token type rather than token text (this is just for efficiency, not actually necessary for fixing the main issue)
  • add more trailing token test cases to gUnitHQLGrammar.testsuite

Jira: https://hibernate.atlassian.net/browse/HQLPARSER-39

See also https://hibernate.atlassian.net/browse/HQLPARSER-40

anistor added some commits Feb 26, 2014

HQLPARSER-39 Detection of unconsumed tokens must take non-buffered to…
…kens into account

* ensure the internal token buffer of CommonTokenStream is filled from the underlying TokenSource (lexer) before checking if any tokens are left unmatched
* detect EOF by comparing on token type rather than token text (this is just for efficiency, not actually necessary for fixing the main issue)
* add more trailing token test cases to gUnitHQLGrammar.testsuite

Can one of the admins add this person to the trusted builders?

@@ -102,7 +104,7 @@ private String getUnconsumedTokens(CommonTokenStream tokens) {
for ( Token endToken : (List<Token>) tokens.getTokens( tokens.index(), tokens.size() - 1 ) ) {
// Ignore <EOF> tokens as they might be inserted by the parser
- if ( !"<EOF>".equals( endToken.getText() ) ) {
+ if ( endToken.getType() != Token.EOF ) {
@anistor

anistor Feb 26, 2014

Member

Here we should probably break when EOF is encountered. What's the point of continuing? Can there be more EOFs?

@gunnarmorling

gunnarmorling Feb 27, 2014

Owner

See the comment on the line before; I believe there are cases where these tokens are inserted by the parser. But it has been a while since then and I need to take a closer look.

@anistor

anistor Feb 27, 2014

Member

I saw it; was just wondering if it makes sense. EOF is inserted by the lexer at end of stream. In theory an antlr user that performs token rewriting could insert arbitrary tokens (even eof) anywere. But why would you do it with EOF? Does hibernate parser do it?

@gunnarmorling

gunnarmorling Mar 5, 2014

Owner

I just tried to remove the if-block and indeed some tests fail. I'm not exactly sure why it happens, apparently the generated parser code polls (in some cases) another time for more tokens from the lexer when it sees an EOF. I don't think it's something to be worried about.

@@ -182,7 +184,7 @@ private String getUnconsumedTokens(CommonTokenStream tokens) {
for ( Token endToken : (List<Token>) tokens.getTokens( tokens.index(), tokens.size() - 1 ) ) {
// Ignore <EOF> tokens as they might be inserted by the parser
- if ( !"<EOF>".equals( endToken.getText() ) ) {
+ if ( endToken.getType() != Token.EOF ) {
@anistor

anistor Feb 26, 2014

Member

Same comment here as for QueryParser.getUnconsumedTokens().

Member

anistor commented Mar 4, 2014

Hi @gunnarmorling,
Any chance to integrate this?
I can revert those EOF related changes if we're not sure whether they are ok or not.

Thanks!

Owner

gunnarmorling commented Mar 4, 2014

@anistor Sorry for the delay. I'll check it out asap.

Owner

gunnarmorling commented Mar 5, 2014

@anistor I'm closing this one as I created #22 which contains your original change plus a fix for HQLPARSER-40, making your second commit largely obsolete.

Owner

gunnarmorling commented Mar 5, 2014

Thanks btw. for fixing HQLPARSER-39 :)

Member

anistor commented Mar 5, 2014

Thanks a lot!

@anistor anistor deleted the anistor:HQLPARSER-39 branch Mar 16, 2014

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