Skip to content

Commit

Permalink
fix: reduce [manual_memcpy] indexing when array length is same to l…
Browse files Browse the repository at this point in the history
…oop range

Format

refactor: extract function to shrink function length

fix: remove cmp to calculate range
  • Loading branch information
granddaifuku committed Nov 14, 2023
1 parent ba43632 commit cb528e7
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 8 deletions.
49 changes: 43 additions & 6 deletions clippy_lints/src/loops/manual_memcpy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,9 @@ fn build_manual_memcpy_suggestion<'tcx>(
let dst_base_str = snippet(cx, dst.base.span, "???");
let src_base_str = snippet(cx, src.base.span, "???");

let dst = if dst_offset == sugg::EMPTY && dst_limit == sugg::EMPTY {
let dst = if (dst_offset == sugg::EMPTY && dst_limit == sugg::EMPTY)
|| is_array_length_equal_to_range(cx, start, end, dst.base)
{
dst_base_str
} else {
format!("{dst_base_str}[{}..{}]", dst_offset.maybe_par(), dst_limit.maybe_par()).into()
Expand All @@ -186,11 +188,13 @@ fn build_manual_memcpy_suggestion<'tcx>(
"clone_from_slice"
};

format!(
"{dst}.{method_str}(&{src_base_str}[{}..{}]);",
src_offset.maybe_par(),
src_limit.maybe_par()
)
let src = if is_array_length_equal_to_range(cx, start, end, src.base) {
src_base_str
} else {
format!("{src_base_str}[{}..{}]", src_offset.maybe_par(), src_limit.maybe_par()).into()
};

format!("{dst}.{method_str}(&{src});")
}

/// a wrapper of `Sugg`. Besides what `Sugg` do, this removes unnecessary `0`;
Expand Down Expand Up @@ -446,3 +450,36 @@ fn get_loop_counters<'a, 'tcx>(
.into()
})
}

fn is_array_length_equal_to_range(cx: &LateContext<'_>, start: &Expr<'_>, end: &Expr<'_>, arr: &Expr<'_>) -> bool {
fn extract_lit_value(expr: &Expr<'_>) -> Option<u128> {
if_chain! {
if let ExprKind::Lit(lit) = expr.kind;
if let ast::LitKind::Int(value, _) = lit.node;
then {
Some(value)
} else {
None
}
}
}

let arr_ty = cx.typeck_results().expr_ty(arr).peel_refs();

if let ty::Array(_, s) = arr_ty.kind() {
let size: u128 = if let Some(size) = s.try_eval_target_usize(cx.tcx, cx.param_env) {
size.into()
} else {
return false;
};

let range = match (extract_lit_value(start), extract_lit_value(end)) {
(Some(start_value), Some(end_value)) => end_value - start_value,
_ => return false,
};

size == range
} else {
false
}
}
20 changes: 20 additions & 0 deletions tests/ui/manual_memcpy/without_loop_counters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,26 @@ pub fn manual_copy(src: &[i32], dst: &mut [i32], dst2: &mut [i32]) {
for i in 0..dst.len() {
dst[i] = src[i];
}

// Range is equal to array length
let src = [0, 1, 2, 3, 4];
let mut dst = [0; 4];
for i in 0..4 {
//~^ ERROR: it looks like you're manually copying between slices
dst[i] = src[i];
}

let mut dst = [0; 6];
for i in 0..5 {
//~^ ERROR: it looks like you're manually copying between slices
dst[i] = src[i];
}

let mut dst = [0; 5];
for i in 0..5 {
//~^ ERROR: it looks like you're manually copying between slices
dst[i] = src[i];
}
}

#[warn(clippy::needless_range_loop, clippy::manual_memcpy)]
Expand Down
31 changes: 29 additions & 2 deletions tests/ui/manual_memcpy/without_loop_counters.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ LL | / for i in 0..5 {
LL | |
LL | | dst[i - 0] = src[i];
LL | | }
| |_____^ help: try replacing the loop by: `dst[..5].copy_from_slice(&src[..5]);`
| |_____^ help: try replacing the loop by: `dst[..5].copy_from_slice(&src);`

error: it looks like you're manually copying between slices
--> $DIR/without_loop_counters.rs:121:5
Expand All @@ -120,11 +120,38 @@ LL | | }
error: it looks like you're manually copying between slices
--> $DIR/without_loop_counters.rs:145:5
|
LL | / for i in 0..4 {
LL | |
LL | | dst[i] = src[i];
LL | | }
| |_____^ help: try replacing the loop by: `dst.copy_from_slice(&src[..4]);`

error: it looks like you're manually copying between slices
--> $DIR/without_loop_counters.rs:151:5
|
LL | / for i in 0..5 {
LL | |
LL | | dst[i] = src[i];
LL | | }
| |_____^ help: try replacing the loop by: `dst[..5].copy_from_slice(&src);`

error: it looks like you're manually copying between slices
--> $DIR/without_loop_counters.rs:157:5
|
LL | / for i in 0..5 {
LL | |
LL | | dst[i] = src[i];
LL | | }
| |_____^ help: try replacing the loop by: `dst.copy_from_slice(&src);`

error: it looks like you're manually copying between slices
--> $DIR/without_loop_counters.rs:165:5
|
LL | / for i in 0..src.len() {
LL | |
LL | | dst[i] = src[i].clone();
LL | | }
| |_____^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[..]);`

error: aborting due to 13 previous errors
error: aborting due to 16 previous errors

0 comments on commit cb528e7

Please sign in to comment.