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

implemented lint unnecessary_min #11951

Closed
wants to merge 56 commits into from
Closed

implemented lint unnecessary_min #11951

wants to merge 56 commits into from

Conversation

FelixMaetzler
Copy link

fixes #11924

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: [`unnecessary_min`]: added lint

@rustbot
Copy link
Collaborator

rustbot commented Dec 11, 2023

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

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 11, 2023
@FelixMaetzler
Copy link
Author

Hi, this is my first PR ever so please give me feedback if i did something wrong.
I saw that in the naming conventions is written that such a lint should be named unused but in this case i think unnecessary should fit better.
But i can change it if necessary.

tests/ui/unnecessary_min.rs Outdated Show resolved Hide resolved
tests/ui/unnecessary_min.rs Outdated Show resolved Hide resolved
FelixMaetzler and others added 3 commits December 13, 2023 15:55
Co-authored-by: Marti Raudsepp <marti@juffo.org>
Co-authored-by: Marti Raudsepp <marti@juffo.org>
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_min.rs Outdated Show resolved Hide resolved
Comment on lines +66 to +68
let cv = constant(cx, cx.typeck_results(), expr)?;

match (ty.kind(), cv) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Oh i didnt know this method existed.
I will look into that.
Thank you!

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure how to implement that change.
I need to use the ty.kind() because i have to know how many bits the integer has.
If i use your proposal, than i only know if it is signed or unsigned and i can't use the math in the arms because it gives me a i128 if it is signed. (unsext wants u128)
Do i miss or misunderstand something here?
Thank you for your feedback. I appreciate it.

@bors
Copy link
Collaborator

bors commented Dec 16, 2023

☔ The latest upstream changes (presumably #11977) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Dec 17, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2023

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

@FelixMaetzler
Copy link
Author

i tried the rebase commands as stated above but nothing happens. I get the message: Everything is up-to-date.
Can somebody please help me with what i have to do, to make it happen?

@FelixMaetzler
Copy link
Author

@rustbot ready

@bors
Copy link
Collaborator

bors commented Dec 26, 2023

☔ The latest upstream changes (presumably #12004) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot
Copy link
Collaborator

rustbot commented Dec 29, 2023

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

@rustbot
Copy link
Collaborator

rustbot commented Dec 29, 2023

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

@FelixMaetzler
Copy link
Author

FelixMaetzler commented Dec 29, 2023

can somebody please help me?
i think i broke stuff.
I tried the things which i was asked for but i don't think, that i did it right.
I'm Sorry

@cocodery
Copy link
Contributor

You can try git reset --soft master locally,
sync master branch on github and then pull and merge to this branch,
last, commit your change will be fine.

You can checkout -b a new branch try first, I usually use this method to avoid confict.

@rustbot
Copy link
Collaborator

rustbot commented Dec 30, 2023

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

@FelixMaetzler FelixMaetzler deleted the merge-conflicts branch December 30, 2023 13:58
@FelixMaetzler FelixMaetzler restored the merge-conflicts branch December 30, 2023 13:59
@FelixMaetzler
Copy link
Author

i tried it but i didnt figure it out.
Now i did a new PR #12061 and hope that that works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) 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.

unnecessary call to min function