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

[swap] lints now check if there is no_std or no_core attribute #7877

Merged
merged 3 commits into from
Nov 11, 2021

Conversation

dswij
Copy link
Member

@dswij dswij commented Oct 25, 2021

Closes #7858

changelog: [swap] lints now check if there is no_std or no_core attribute

@rust-highfive
Copy link

r? @camsteffen

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 25, 2021
clippy_lints/src/swap.rs Outdated Show resolved Hide resolved
""
} else {
"core"
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use one more util std_or_core() -> Option<&'static str>. Shouldn't use empty strings as "none"`.

Copy link
Member Author

@dswij dswij Nov 1, 2021

Choose a reason for hiding this comment

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

Was going for a direct return there on the last else statement, but I did this as a workaround since if_not_else is warning this

    let sugg = if !is_no_std_crate(cx) {
        "std"
    } else if !is_no_core_crate(cx) {
        "core"
    } else {
        return;
    };

Now that #7895 is merged, I think we can go for the above, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think std_or_core would be good to have in clippy_utils. You could sneak in a let-else.

let Some(sugg) = std_or_core(cx) else { return };

Copy link
Member Author

Choose a reason for hiding this comment

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

let-else sounds amazing

Copy link
Member Author

@dswij dswij Nov 2, 2021

Choose a reason for hiding this comment

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

Oh whoops, semicolon-if-nothing-returned FP on this?

let Some(sugg) = std_or_core(cx) else { return };

produces

   --> src/swap.rs:116:5
    |
116 |     let Some(sugg) = std_or_core(cx) else { return };
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add a `;` here: `let Some(sugg) = std_or_core(cx) else { return };;`
    |
    = note: `-D clippy::semicolon-if-nothing-returned` implied by `-D clippy::pedantic`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#semicolon_if_nothing_returned

I will open an issue for this

@camsteffen camsteffen added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 31, 2021
@dswij dswij requested a review from camsteffen November 2, 2021 03:44
Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

LGTM but we need to do something about the false positive in the CI build. Would you be interested in fixing that in another PR?

Moved out reusable pieces from `is_automatically_derived` and
`any_parent_is_automatically_derived`.
@dswij
Copy link
Member Author

dswij commented Nov 11, 2021

FP in CI build should be fixed by #7955 🙂

@camsteffen
Copy link
Contributor

Nice. Can you squash some commits?

This commit adds a `no_std` and `no_core` check on `swap` lint and additionally suggest `core::mem::swap` whenever possible.
Remove warning if both `std` and `core` is not present.
Adds additional test to check for `swap` suggestion when `no_std` is present
@dswij
Copy link
Member Author

dswij commented Nov 11, 2021

Nice. Can you squash some commits?

Done

@camsteffen
Copy link
Contributor

@bors r+ thanks!

@bors
Copy link
Collaborator

bors commented Nov 11, 2021

📌 Commit dcd1a16 has been approved by camsteffen

@bors
Copy link
Collaborator

bors commented Nov 11, 2021

⌛ Testing commit dcd1a16 with merge 8389df9...

@bors
Copy link
Collaborator

bors commented Nov 11, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 8389df9 to master...

@bors bors merged commit 8389df9 into rust-lang:master Nov 11, 2021
@dswij dswij deleted the no-std-fp branch November 11, 2021 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clippy suggests std::foo with #[!no_std] being enabled
4 participants