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

Tidy up go runtime #3186

Closed
wants to merge 1 commit into from
Closed

Tidy up go runtime #3186

wants to merge 1 commit into from

Conversation

Joakker
Copy link
Contributor

@Joakker Joakker commented May 23, 2021

Partially addresses #2504 (comment)

This PR normalizes the doc comments in the go runtime sources to one more compatible with the go documentation tool, godoc, so the auto-generated docs over at pkg.go.dev look more readable, since people familiar with go will look there first for the documentation instead of in this repo.

It also hides some elements that don't need to be exported as they don't have to do strictly with the generated parser.

@Joakker Joakker marked this pull request as draft May 23, 2021 19:14
@Joakker
Copy link
Contributor Author

Joakker commented Jun 10, 2021

Aight, the go tests are passing, so that's what matters

@Joakker Joakker marked this pull request as ready for review June 10, 2021 22:52
@Joakker
Copy link
Contributor Author

Joakker commented Jul 5, 2021

cc @parrt

@parrt
Copy link
Member

parrt commented Jul 5, 2021

Maybe @davesisson @pboyer @willfaught could take a look and bless the changes.

@Joakker
Copy link
Contributor Author

Joakker commented Jul 5, 2021

Okie, thanks

Copy link
Contributor

@willfaught willfaught left a comment

Choose a reason for hiding this comment

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

I got a few files in and then noticed how many changes there are. I won't be able to cover it all in one day. Hopefully the others can pitch in too. :)

runtime/Go/antlr/atn.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config_set.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config_set.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config_set.go Show resolved Hide resolved
runtime/Go/antlr/atn_deserialization_options.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config_set.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config_set.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config_set.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config_set.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config_set.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config_set.go Outdated Show resolved Hide resolved
runtime/Go/antlr/atn_config_set.go Outdated Show resolved Hide resolved
@Joakker
Copy link
Contributor Author

Joakker commented Aug 8, 2021

Aight, that should do it

@parrt
Copy link
Member

parrt commented Aug 9, 2021

Hi @Joakker it looks like we are getting a Go language failure:

testOne[Go:JavaExpressions_8](org.antlr.v4.test.runtime.go.TestLeftRecursion)  Time elapsed: 0.98 sec  <<< FAILURE!
junit.framework.AssertionFailedError: 
[Go:JavaExpressions_8] Parse output is incorrect: expectedOutput:<(s (e (e new (typespec A) ( )) . b) <EOF>)
>; actualOutput:<>; expectedParseErrors:<>; actualParseErrors:<# antlr.org/test/parser/antlr
parser\antlr\semantic_context.go:163:55: undefined: _set
>; expectedToolErrors:<>; actualToolErrors:<>.

@Joakker
Copy link
Contributor Author

Joakker commented Aug 10, 2021

Oop, will fix in the morning

return NewBaseATNConfig(c, c.GetState(), c.GetContext(), semanticContext)
}

func NewBaseATNConfig1(c ATNConfig, state ATNState, context PredictionContext) *BaseATNConfig {
// ATNConfigWithStateContext creates a new instance of BaseATNConfig.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if these comments actually add anything. I don't know go very well at all but it seems like these comments are not adding very much to constructor functions.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, they look like useless comments because they don't give additional information, don't explain how an algorithm works, etc.

@parrt
Copy link
Member

parrt commented Dec 28, 2021

seems like a huge change; perhaps @jcking or someone more knowledgeable can scan through but many of those comments seem unnecessary to me per my comment I just made.

@parrt parrt changed the base branch from master to dev March 27, 2022 18:07
@parrt
Copy link
Member

parrt commented Apr 2, 2022

Guys trying to clean up. I think I'll close as there is controversy here. sorry!

@parrt parrt closed this Apr 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants