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

Catch pointer::cast too in cast_ptr_alignment #6557

Merged
merged 3 commits into from
Jan 11, 2021

Conversation

rail-rain
Copy link
Contributor

Fixes #4708

Although there were some discussion in the issue, this PR implements the original feature. I think cast_ptr_alignment should exist as it is, separated from ptr_as_ptr.


changelog: Extend cast_ptr_alignment lint for the pointer::cast method

@rust-highfive
Copy link

r? @phansch

(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 Jan 7, 2021
Copy link
Member

@phansch phansch left a comment

Choose a reason for hiding this comment

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

Looks good to me in general, just got some minor improvements/refactorings =)

clippy_lints/src/types.rs Outdated Show resolved Hide resolved
@@ -1691,6 +1687,21 @@ impl<'tcx> LateLintPass<'tcx> for Casts {
lint_numeric_casts(cx, expr, ex, cast_from, cast_to);
}

lint_cast_ptr_alignment(cx, expr, cast_from, cast_to);
} else if let ExprKind::MethodCall(method_path, _, args, _) = expr.kind {
if method_path.ident.name != sym!(cast) {
Copy link
Member

Choose a reason for hiding this comment

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

This condition can also be a part of the if_chain below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm not sure if I know what you mean.

Does it look like this:

if let ExprKind::Cast(ref ex, cast_to) = expr.kind {
    ...
    lint_cast_ptr_alignment(cx, expr, cast_from, cast_to);
} else if let ExprKind::MethodCall(method_path, _, args, _) = expr.kind {
    if_chain! {
        if method_path.ident.name == sym!(cast);
        if let Some(generic_args) = method_path.args;
        if let [GenericArg::Type(cast_to)] = generic_args.args;
        // There probably is no obvious reason to do this, just to be consistent with `as` cases.
        if !is_hir_ty_cfg_dependant(cx, cast_to);
        then {
            let (cast_from, cast_to) =
                (cx.typeck_results().expr_ty(&args[0]), cx.typeck_results().expr_ty(expr));
            lint_cast_ptr_alignment(cx, expr, cast_from, cast_to);
        }
    }

or this?:

if let ExprKind::Cast(ref ex, cast_to) = expr.kind {
    ...
    lint_cast_ptr_alignment(cx, expr, cast_from, cast_to);
}

if_chain! {
    if let ExprKind::MethodCall(method_path, _, args, _) = expr.kind;
    if method_path.ident.name == sym!(cast);
    if let Some(generic_args) = method_path.args;
    if let [GenericArg::Type(cast_to)] = generic_args.args;
    // There probably is no obvious reason to do this, just to be consistent with `as` cases.
    if !is_hir_ty_cfg_dependant(cx, cast_to);
    then {
        let (cast_from, cast_to) = (cx.typeck_results().expr_ty(&args[0]), cx.typeck_results().expr_ty(expr));
        lint_cast_ptr_alignment(cx, expr, cast_from, cast_to);
    }
}

Both look better than the current one though.

Copy link
Member

Choose a reason for hiding this comment

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

I had this in mind:

if let ExprKind::Cast(ref ex, cast_to) = expr.kind {
    ...
    lint_cast_ptr_alignment(cx, expr, cast_from, cast_to);
} else if let ExprKind::MethodCall(method_path, _, args, _) = expr.kind {
    if_chain! {
        if method_path.ident.name != sym!(cast);
        if let Some(generic_args) = method_path.args;
        if let [GenericArg::Type(cast_to)] = generic_args.args;
        // There probably is no obvious reason to do this, just to be consistent with `as` cases.
        if is_hir_ty_cfg_dependant(cx, cast_to);
        then {
            return;
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, my guess was not so far away. Since there will be only one if_chain!, I think my first one is the best. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes I must have misunderstood it the first time, that's indeed better =)

tests/ui/cast_alignment.rs Show resolved Hide resolved
add stuff on pointer::cast` to the document for `cast_ptr_alignment`
and fix line numbers in the test.
@phansch phansch 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 Jan 9, 2021
@phansch
Copy link
Member

phansch commented Jan 11, 2021

@bors r+ thanks!

@bors
Copy link
Contributor

bors commented Jan 11, 2021

📌 Commit 53f8731 has been approved by phansch

@bors
Copy link
Contributor

bors commented Jan 11, 2021

⌛ Testing commit 53f8731 with merge 7f4599a...

@bors
Copy link
Contributor

bors commented Jan 11, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: phansch
Pushing 7f4599a to master...

@bors bors merged commit 7f4599a into rust-lang:master Jan 11, 2021
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.

Extend cast_ptr_alignment lint for pointer::cast method
4 participants