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

feat(java 21): allow toplevel method declarations #160

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

brandonspark
Copy link
Contributor

@brandonspark brandonspark commented Aug 16, 2023

What:

This PR allows top-level statements to comprise of method declarations.

Why:

This is because Java 21 adds "unnamed classes", which means such classes can now be "anonymous", and now method declarations can just exist at the top level. This is a Java 21 feature that the grammar should aim to accommodate.

How:

Just made a new hidden nonterminal called toplevel_statements, which may be a method_declaration.

I could have changed statement itself, but statement is used at places that are not the top level, and this causes conflicts.

Worth noting that the state count increases by about 100. That's not a whole lot, so I'm cool with it, but worth noting.

Test plan:

tree-sitter test

Checklist:

  • All tests pass in CI.
  • There are sufficient tests for the new fix/feature.
  • Grammar rules have not been renamed unless absolutely necessary.
  • The conflicts section hasn't grown too much.
  • The parser size hasn't grown too much (check the value of STATE_COUNT in src/parser.c).

@amaanq
Copy link
Member

amaanq commented Aug 17, 2023

Can you combine all your PRs into one PR, separate the changes by commit instead, then do one generate at the end as its own commit? This way the repo isn't unnecessarily bloated with changes to parser.c

@aryx
Copy link
Contributor

aryx commented Aug 17, 2023

To be honest I prefer to have separate PRs for each feature added.
The problem is that we should not have those generated parser.c code in the repo in the first place ... They make the reviewing process more difficult. I sometimes can't even review PR on github because the diff is 1M LOC.
I don't know when @maxbrunsfeld will decide we can remove those generated files.

@amaanq
Copy link
Member

amaanq commented Aug 17, 2023

To be honest I prefer to have separate PRs for each feature added. The problem is that we should not have those generated parser.c code in the repo in the first place ... They make the reviewing process more difficult. I sometimes can't even review PR on github because the diff is 1M LOC. I don't know when @maxbrunsfeld will decide we can remove those generated files.

I can agree w/ having a PR per feature, but the issue is when each PR has generated files it unnecessarily adds to the repo size when one generation after all the PRs would be much better.

We can't remove the files for now, ideally using something like GH artifacts to host generated files would be best, but we're not there yet

@brandonspark
Copy link
Contributor Author

@aryx this can be merged now

@aryx aryx merged commit ca4afaa into tree-sitter:master Aug 28, 2023
3 checks passed
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