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

New comparison_negation_linter #2272

Merged
merged 2 commits into from
Nov 12, 2023
Merged

New comparison_negation_linter #2272

merged 2 commits into from
Nov 12, 2023

Conversation

MichaelChirico
Copy link
Collaborator

@MichaelChirico MichaelChirico commented Nov 10, 2023

Part of #884

No lints on our own package

@AshesITR
Copy link
Collaborator

Out of curiosity:
Is there some S3 magic where the negation makes sense?
Thinking set containment relations that might be implemented via Ops.

@MichaelChirico
Copy link
Collaborator Author

MichaelChirico commented Nov 11, 2023

That's absolutely something that could happen in principal, but it's definitely a code smell if so.

In fact, C++20 introduced compiler-level changes to prevent this from becoming an issue -- if you write an == method, you get != for free because the compiler rewrites x!=y to !(x==y). They also added the spaceship operator <=> for similar reasons re: >,,<,:

https://stackoverflow.com/a/60177760/3576984

If you wrote it this way because of dispatch issues in upstream code, # nolint would be the way to go.

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

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

Do we want to add it to our .lintr_new?

@MichaelChirico MichaelChirico merged commit 865e2e1 into main Nov 12, 2023
20 checks passed
@MichaelChirico MichaelChirico deleted the comparison_negation branch November 12, 2023 17:26
@MichaelChirico
Copy link
Collaborator Author

Do we want to add it to our .lintr_new?

We might want to switch to something like modify_defaults(all_linters(), ...) for our .lintr TBH...

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.

2 participants