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 lint: Suggest `ptr.add([usize])` over `ptr.offset([usize] as isize)`. #3104

Merged
merged 12 commits into from Aug 29, 2018

Conversation

Projects
None yet
3 participants
@frewsxcv
Member

frewsxcv commented Aug 29, 2018

First part of #3047.

let vec = vec![b'a', b'b', b'c'];
let ptr = vec.as_ptr();
let offset = 1_usize;

unsafe { ptr.offset(offset as isize); }

Could be written:

let vec = vec![b'a', b'b', b'c'];
let ptr = vec.as_ptr();
let offset = 1_usize;

unsafe { ptr.add(offset); }

@frewsxcv frewsxcv force-pushed the frewsxcv:frewsxcv-ptr-offset-with-cast branch from f8b2da3 to e0c7eb7 Aug 29, 2018

/// ```
declare_clippy_lint! {
pub PTR_OFFSET_WITH_CAST,
style,

This comment has been minimized.

@frewsxcv

frewsxcv Aug 29, 2018

Member

Is this the right category?

This comment has been minimized.

@oli-obk

oli-obk Aug 29, 2018

Collaborator

It would fit the complexity category. Feels less like a stylistic choice, more like in the scope of "use x.is_empty() instead of x.len() == 0.

This comment has been minimized.

@frewsxcv

@frewsxcv frewsxcv force-pushed the frewsxcv:frewsxcv-ptr-offset-with-cast branch from e0c7eb7 to 5ebae01 Aug 29, 2018

@oli-obk

That split into many functions <3

Just a few small things. Also: you can probably easily extend your code to also catch wrapping_offset doing the same thing

cx: &lint::LateContext<'a, 'tcx>,
expr: &hir::Expr,
) -> bool {
cx.tables.expr_ty(expr).sty == ty::TyKind::Uint(ast::UintTy::Usize)

This comment has been minimized.

@oli-obk

oli-obk Aug 29, 2018

Collaborator

you can use cx.tcx.types.usize to compare directly with the result of expr_ty

This comment has been minimized.

@frewsxcv
cx: &lint::LateContext<'a, 'tcx>,
expr: &hir::Expr,
) -> bool {
if let ty::RawPtr(..) = cx.tables.expr_ty(expr).sty {

This comment has been minimized.

@oli-obk

oli-obk Aug 29, 2018

Collaborator

You can use the is_unsafe_ptr method instead of destructuring

This comment has been minimized.

@frewsxcv
utils::snippet_opt(cx, cast_lhs_expr.span)
) {
(Some(receiver), Some(cast_lhs)) => format!("{}.add({})", receiver, cast_lhs),
_ => String::new(),

This comment has been minimized.

@oli-obk

oli-obk Aug 29, 2018

Collaborator

if the snippet is not obtainable, instead of reporting an empty snippet, we should not produce any suggestion at all

This comment has been minimized.

@frewsxcv
@frewsxcv

This comment has been minimized.

Member

frewsxcv commented Aug 29, 2018

thanks for the quick review @oli-obk! your comments should be addressed

@frewsxcv

This comment has been minimized.

Member

frewsxcv commented Aug 29, 2018

Just a few small things. Also: you can probably easily extend your code to also catch wrapping_offset doing the same thing

oh missed this, let me add it in

}
fn build_suggestion<'a, 'tcx>(
cx: &lint::LateContext<'a, 'tcx>,
receiver_expr: &hir::Expr,
cast_lhs_expr: &hir::Expr,
) -> String {
) -> Option<String> {
match (
utils::snippet_opt(cx, receiver_expr.span),

This comment has been minimized.

@oli-obk

oli-obk Aug 29, 2018

Collaborator

Now your code can be simplified to

let receiver = utils::snippet_opt(cx, receiver_expr.span)?;
let cast_lhs = utils::snippet_opt(cx, cast_lhs_expr.span)?;
Some(format!("{}.add({})", receiver, cast_lhs))

This comment has been minimized.

@frewsxcv

frewsxcv Aug 29, 2018

Member

oh right, ? with Option is now a thing 💅

frewsxcv added some commits Aug 29, 2018

@oli-obk

This comment has been minimized.

Collaborator

oli-obk commented Aug 29, 2018

travis is unhappy (just make your enum Copy).

r=me with travis passing

@mati865

This comment has been minimized.

Contributor

mati865 commented Aug 29, 2018

Travis error:

error: this argument is passed by reference, but would be more efficient if passed by value
   --> clippy_lints/src/ptr_offset_with_cast.rs:136:19
    |
136 |     fn suggestion(&self) -> &'static str {
    |                   ^^^^^ help: consider passing by value instead: `self`
    |
    = note: `-D trivially-copy-pass-by-ref` implied by `-D clippy`
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#trivially_copy_pass_by_ref

frewsxcv added some commits Aug 29, 2018

@frewsxcv frewsxcv force-pushed the frewsxcv:frewsxcv-ptr-offset-with-cast branch from ed0b16a to f42442b Aug 29, 2018

@frewsxcv

This comment has been minimized.

Member

frewsxcv commented Aug 29, 2018

@bors r=oli-obk

@oli-obk

This comment has been minimized.

Collaborator

oli-obk commented Aug 29, 2018

I don't think we have bors setup. we totally should though

@frewsxcv

This comment has been minimized.

Member

frewsxcv commented Aug 29, 2018

wasn't sure if i had bors privileges here – looks like someone else will have to approve or merge

@oli-obk oli-obk merged commit 0f2eab6 into rust-lang:master Aug 29, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@frewsxcv frewsxcv deleted the frewsxcv:frewsxcv-ptr-offset-with-cast branch Aug 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment