Skip to content

Drop no longer needed C++ modification #757

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 13, 2022
Merged

Drop no longer needed C++ modification #757

merged 2 commits into from
Jan 13, 2022

Conversation

@Luni-4
Copy link
Collaborator

@Luni-4 Luni-4 commented Jan 13, 2022 •
edited
Loading

This PR fixes #756 and #49

@Luni-4 Luni-4 requested a review from marco-c January 13, 2022 12:15
@marco-c
Copy link
Collaborator

marco-c commented Jan 13, 2022

Did you try running the minimal tests stuff to see if there are differences?

@marco-c
Copy link
Collaborator

marco-c commented Jan 13, 2022

Maybe we should update the mozcpp version to trigger the test on CI?

calixteman
calixteman approved these changes Jan 13, 2022
View reviewed changes
Copy link
Collaborator

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

It's useless now.

@marco-c
Copy link
Collaborator

marco-c commented Jan 13, 2022

It's useless now.

It seems there are differences in the metrics, so the changes were not fully integrated upstream

@calixteman
Copy link
Collaborator

calixteman commented Jan 13, 2022

Normally it has been, but probably some other changes are responsible for this variations.

@calixteman
Copy link
Collaborator

calixteman commented Jan 13, 2022

I check ts-cpp repo and my changed have been merged.
An other PR superseded them but the tests I wrote for my PR are of course still there:
https://github.com/tree-sitter/tree-sitter-cpp/blob/e8dcc9d2b404c542fd236ea5f7208f90be8a6e89/test/corpus/declarations.txt#L539-L543
So the operator bug is definitely fixed (the code Luni removed in this patch aimed to fix some issues with whitespaces in operator definition).
So from my pov, this PR is safe, or I missed something ;).

@marco-c
Copy link
Collaborator

marco-c commented Jan 13, 2022

All right, sounds good then.

@marco-c marco-c merged commit 3eb8960 into master Jan 13, 2022
@marco-c marco-c deleted the remove-grammar branch January 13, 2022 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@calixteman calixteman calixteman approved these changes

@marco-c marco-c Awaiting requested review from marco-c

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Drop no longer needed modification of the C++ grammar

4 participants

@Luni-4 @marco-c @calixteman