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(fe): warn on confusing operator precedence of & and >> #1093

Closed
wants to merge 3 commits into from

Conversation

toastin0
Copy link
Contributor

@toastin0 toastin0 commented Oct 15, 2023

closes #1056
i'm excited to hear any feedback you have, so please, let me know!

@github-actions
Copy link

github-actions bot commented Oct 15, 2023

CLA Assistant Lite bot Thank you for your contribution! Like many free software projects, you must sign our Contributor License Agreement before we can accept your contribution.

EDIT: All contributors have signed quick-lint-js' Contributor License Agreement (CLA-v1.md).

@toastin0
Copy link
Contributor Author

I have read and hereby agree to quick-lint-js' Contributor License Agreement (CLA-v1.md).

Copy link
Collaborator

@strager strager left a comment

Choose a reason for hiding this comment

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

This patch looks good. Let me know if you want to make changes or if I should merge this as-is.

docs/errors/E0715.md Outdated Show resolved Hide resolved
docs/errors/E0715.md Outdated Show resolved Hide resolved
docs/errors/E0715.md Outdated Show resolved Hide resolved
src/quick-lint-js/diag/diagnostic-types-2.h Outdated Show resolved Hide resolved
.bitshift_operator = right_op});
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, clean logic. 👍

@strager
Copy link
Collaborator

strager commented Oct 25, 2023

@toastin0 Is this ready to be merged?

@toastin0
Copy link
Contributor Author

toastin0 commented Oct 27, 2023

sorry, was working on a commit but then life got in the way :(
I should have the second message added in tomorrow, will let you know!

@toastin0
Copy link
Contributor Author

toastin0 commented Oct 28, 2023

lmk if there are any issues with the latest commit, and if not, should be good to merge!

@toastin0
Copy link
Contributor Author

oh hey, just looked, and saw that there's already been an E0715 added. should I update it with a new error number, or will you do that while merging?

@strager
Copy link
Collaborator

strager commented Nov 2, 2023

@toastin0 I will fix the conflicts (including renumbering diagnostic codes) if you don't. No worries.

@toastin0
Copy link
Contributor Author

toastin0 commented Nov 2, 2023

cool! then if there are no further issues with the commit, then we're good to merge :3

@strager
Copy link
Collaborator

strager commented Nov 3, 2023

Landed as Git commit aaf8db6. I renamed E0715 to E0716, fixed an issue with the E0716.md examples, and ran clang-format on the code.

@strager strager closed this Nov 3, 2023
@toastin0 toastin0 deleted the add-diag-E0715 branch November 9, 2023 16:07
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.

10$: Warn on mixing >> and & precedence
2 participants