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

Fixes #217

Merged
merged 9 commits into from
Jul 27, 2023
Merged

Fixes #217

merged 9 commits into from
Jul 27, 2023

Conversation

amaanq
Copy link
Member

@amaanq amaanq commented Jul 25, 2023

Did this to prep for another release, and most of these were easy enough to fix and aren't anything complicated.

The main change that really increased state count a bit is backporting the C updates where I added attribute_specifier to more allowed locations.

Closes #47
Closes #63 (really a C bug fixed in 0.20.4)
Closes #96
Closes #102
Closes #103
Closes #112
Closes #139
Closes #203

The last issue that's a bit difficult is the parenthesized assignment expressions, that's because of fold operators taking precedence over them but I could not figure that out, I'll try a bit more anyways

@jdrouhard
Copy link
Collaborator

I'll take a look at this at some point tomorrow. Thanks for working on this!

grammar.js Outdated Show resolved Hide resolved
@@ -66,6 +62,7 @@ module.exports = grammar(C, {
[$._binary_fold_operator, $._fold_operator],
[$.expression_statement, $.for_statement],
[$.init_statement, $.for_statement],
[$._function_declarator_seq],
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the new conflict here?

Copy link
Member Author

Choose a reason for hiding this comment

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

repeat($.attribute_specifier) after noexcepts

grammar.js Outdated
$.using_declaration,
$.type_definition,
$.static_assert_declaration,
';',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Semicolons can technically appear anywhere a statement is valid. I think you were trying to close a particular issue regarding having an extraneous semicolon (#35?), but I think we should hold off and try to figure out a way to make "empty" statements (a single semicolon) work anywhere a statement could appear (any of which would end with a semicolon), instead of only as a field declaration.

Not sure how terrible that would be for node count though :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, maybe something like javascript's automatic semicolons could be used, only adds 7 to state count though

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not familiar with that, but I think we should make something like an empty_statement: $ => ';' rule and just put that as an option in various places where other statements (including top level) can be. Not the most elegant and I haven't actually tried this so it may not really work in practice but something like that is the best way to make the grammar not choke on extra semicolons.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/tree-sitter/tree-sitter-javascript/blob/master/src/scanner.c, though C++ is a bit different and an empty_statement node would be just fine as well - but I think naming it _empty_statement to hide the rule could be better

Copy link
Collaborator

@jdrouhard jdrouhard left a comment

Choose a reason for hiding this comment

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

Don't forget to update the list of issues this will close since we're punting a couple for another PR.

Looks good

@amaanq amaanq merged commit 994441f into tree-sitter:master Jul 27, 2023
4 checks passed
@DennySun2100
Copy link

Hi @amaanq,

I am asking this just in case I miss it, are you working on the fix for the precedence issue of parenthesized assignment expressions?

@amaanq
Copy link
Member Author

amaanq commented Aug 4, 2023

Thanks for the reminder, it slipped my mind completely. I've come up with a solution now but I'd appreciate more testing for potential pitfalls.
#219

@DennySun2100
Copy link

Thanks for the reminder, it slipped my mind completely. I've come up with a solution now but I'd appreciate more testing for potential pitfalls. #219

It's great to hear you've got a solution, looking forward to seeing the PR soon.

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