Skip to content

Conversation

@kaby76
Copy link
Contributor

@kaby76 kaby76 commented Feb 22, 2022

What does this PR fix?

Discussion

This is a fix for the missing API method String() in the Token, and Reset() in the Lexer.

Go requires any exported data or functions to be capitalized. These functions are required for this code to print out the token stream:

    j := 0
    for {
        t := lexer.NextToken()
        fmt.Print(j)
        fmt.Print(" ")
        // missing fmt.Println(t.String())
        fmt.Println(t.GetText())
        if t.GetTokenType() == antlr.TokenEOF {
            break
        }
        j = j + 1
    }
    // missing lexer.Reset()

IToken.ToString() in C# and Token.toString() in Java are available because whatever implementation is used, it is derived from object. Thus, in C#, you can do t.ToString() or Java, t.toString(). Yet, t.String() in Go causes a compilation error.

Reset() in C# and reset() in Java are explicitly listed public in the interface. Thus, in C#, you can do lexer.Reset();, and Java, lexer.reset();. Yet, lexer.Reset() or lexer.reset() in Go results in a compilation error.

The change is renames two methods. I've included a test in the runtime for the two methods.

Why is this PR important?

grammars-v4 CI tests most of the Antlr targets on a per-commit basis. The CI testing has pointed to many important bugs in Antlr. When a difference in parsing is detected, we need ways to easily compare the computations of the target with other targets. A print out of the tokens during the parse is absolutely essential in discovering why a parser target is not working.

@kaby76 kaby76 changed the title Fix for #3441 and #3443 -- expose Reset() and String() methods. [Go] Fix for #3441 and #3443 -- expose Reset() and String() methods. Feb 22, 2022
next(t, lexer, "[@-1,9:9=' ',<7>,1:9]")
next(t, lexer, "[@-1,10:10='3',<2>,1:10]")
next(t, lexer, "[@-1,11:10='<EOF>',<-1>,1:11]")
}
Copy link
Member

Choose a reason for hiding this comment

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

Should it be a test on Reset method as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, looks like I missed that. Thanks.

@kaby76
Copy link
Contributor Author

kaby76 commented Feb 23, 2022

Waiting for #3558

@jimidle
Copy link
Collaborator

jimidle commented Mar 4, 2023

Guys - I have fixed this in the /v4 code, where it should be - I used this code and added the new test file (though really, those test files are useless and I need to actually add some tests. The build tests are adequate for now, but there is supposed to be a test suite in the actual go runtime itself . What is there right now is just a few half-hearted tests that don't really help anything.

Anyway, you can close this PR now as I have used @kaby76 Ken's code and put it in the /v4 code. That PR will come in shortly and close the issues that this PR was correctly addressing.

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