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

[erasing_op] lint ignored when operation Output type is different from the type of constant 0 #8204

Merged
merged 1 commit into from
Jan 2, 2022
Merged

[erasing_op] lint ignored when operation Output type is different from the type of constant 0 #8204

merged 1 commit into from
Jan 2, 2022

Conversation

wigy-opensource-developer
Copy link
Contributor

@wigy-opensource-developer wigy-opensource-developer commented Dec 31, 2021

fixes #7210

changelog: [erasing_op] lint ignored when operation Output type is different from the type of constant 0

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 31, 2021
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Welcome to Clippy 👋. Your changes look good to me, I only have two smaller comments.

I've also looked at the implementation of identity_op which restricts the lint emissions to only trigger on number types. I think your solution of comparing the types is the better solution. We might want to think about also making this change to the identity_op implementation 🙃

clippy_lints/src/erasing_op.rs Outdated Show resolved Hide resolved
tests/ui/erasing_op.rs Show resolved Hide resolved
@wigy-opensource-developer
Copy link
Contributor Author

Implemented based on the suggestion, squashed and rebased on master.

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

LGTM

@xFrednet
Copy link
Member

xFrednet commented Jan 2, 2022

Looks good to me, thank you for the swift response, could you bless the tests one more time and squash it, then it's ready to be merged! 🙃

@wigy-opensource-developer
Copy link
Contributor Author

Aye, sir, just noticed the breakage. Fixed.

@xFrednet
Copy link
Member

xFrednet commented Jan 2, 2022

Alright, looks good to me, I hope you also had fun while working on this 🙃

@bors r+

@bors
Copy link
Contributor

bors commented Jan 2, 2022

📌 Commit e8b6b2a has been approved by xFrednet

@bors
Copy link
Contributor

bors commented Jan 2, 2022

⌛ Testing commit e8b6b2a with merge 07326fe...

bors added a commit that referenced this pull request Jan 2, 2022
[`erasing_op`] lint ignored when operation `Output` type is different from the type of constant `0`

fixes #7210

changelog:
[`erasing_op`] lint ignored when operation `Output` type is different from the type of constant `0`
@bors
Copy link
Contributor

bors commented Jan 2, 2022

💔 Test failed - checks-action_test

@xFrednet
Copy link
Member

xFrednet commented Jan 2, 2022

The changelog label and entry have to be on the same line, it should work now 🙃

@bors retry

@bors
Copy link
Contributor

bors commented Jan 2, 2022

⌛ Testing commit e8b6b2a with merge 8419108...

@wigy-opensource-developer
Copy link
Contributor Author

wigy-opensource-developer commented Jan 2, 2022

Oh. Thanks. Sure, I enjoyed it. First time I looked below the hood and it was less scary than I thought. I only made a Roslyn check for C# before, that was worse 🤣

@bors
Copy link
Contributor

bors commented Jan 2, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 8419108 to master...

@bors bors merged commit 8419108 into rust-lang:master Jan 2, 2022
@xFrednet
Copy link
Member

xFrednet commented Jan 2, 2022

That's good to hear, the manageable code base was actually the reason why I started to contribute to Clippy. Rustc and other compiler projects seemed to intimidating xD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

erasing_op and identity_op should only be triggered for internal
4 participants