diff --git a/README.md b/README.md index 3f3ed135e0ce..1f6fb4ff9c0b 100644 --- a/README.md +++ b/README.md @@ -256,7 +256,7 @@ name [iter_next_loop](https://github.com/Manishearth/rust-clippy/wiki#iter_next_loop) | warn | for-looping over `_.next()` which is probably not intended [iter_nth](https://github.com/Manishearth/rust-clippy/wiki#iter_nth) | warn | using `.iter().nth()` on a standard library type with O(1) element access [iter_skip_next](https://github.com/Manishearth/rust-clippy/wiki#iter_skip_next) | warn | using `.skip(x).next()` on an iterator -[large_enum_variant](https://github.com/Manishearth/rust-clippy/wiki#large_enum_variant) | warn | large variants on an enum +[large_enum_variant](https://github.com/Manishearth/rust-clippy/wiki#large_enum_variant) | warn | large size difference between variants on an enum [len_without_is_empty](https://github.com/Manishearth/rust-clippy/wiki#len_without_is_empty) | warn | traits or impls with a public `len` method but no corresponding `is_empty` method [len_zero](https://github.com/Manishearth/rust-clippy/wiki#len_zero) | warn | checking `.len() == 0` or `.len() > 0` (or similar) when `.is_empty()` could be used instead [let_and_return](https://github.com/Manishearth/rust-clippy/wiki#let_and_return) | warn | creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block diff --git a/clippy_lints/src/large_enum_variant.rs b/clippy_lints/src/large_enum_variant.rs index 36c5e7a288ae..f656d513b516 100644 --- a/clippy_lints/src/large_enum_variant.rs +++ b/clippy_lints/src/large_enum_variant.rs @@ -1,4 +1,4 @@ -//! lint when there are large variants on an enum +//! lint when there is a large size difference between variants on an enum use rustc::lint::*; use rustc::hir::*; @@ -7,7 +7,7 @@ use rustc::ty::layout::TargetDataLayout; use rustc::ty::TypeFoldable; use rustc::traits::Reveal; -/// **What it does:** Checks for large variants on `enum`s. +/// **What it does:** Checks for large size differences between variants on `enum`s. /// /// **Why is this bad?** Enum size is bounded by the largest variant. Having a large variant /// can penalize the memory layout of that enum. @@ -24,17 +24,17 @@ use rustc::traits::Reveal; declare_lint! { pub LARGE_ENUM_VARIANT, Warn, - "large variants on an enum" + "large size difference between variants on an enum" } #[derive(Copy,Clone)] pub struct LargeEnumVariant { - maximum_variant_size_allowed: u64, + maximum_size_difference_allowed: u64, } impl LargeEnumVariant { - pub fn new(maximum_variant_size_allowed: u64) -> Self { - LargeEnumVariant { maximum_variant_size_allowed: maximum_variant_size_allowed } + pub fn new(maximum_size_difference_allowed: u64) -> Self { + LargeEnumVariant { maximum_size_difference_allowed: maximum_size_difference_allowed } } } @@ -50,6 +50,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LargeEnumVariant { if let ItemEnum(ref def, _) = item.node { let ty = cx.tcx.item_type(did); let adt = ty.ty_adt_def().expect("already checked whether this is an enum"); + + let mut smallest_variant: Option<(_, _)> = None; + let mut largest_variant: Option<(_, _)> = None; + for (i, variant) in adt.variants.iter().enumerate() { let data_layout = TargetDataLayout::parse(cx.sess()); cx.tcx.infer_ctxt((), Reveal::All).enter(|infcx| { @@ -67,32 +71,57 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LargeEnumVariant { } }) .sum(); - if size > self.maximum_variant_size_allowed { - span_lint_and_then(cx, - LARGE_ENUM_VARIANT, - def.variants[i].span, - "large enum variant found", - |db| { - if variant.fields.len() == 1 { - let span = match def.variants[i].node.data { - VariantData::Struct(ref fields, _) | - VariantData::Tuple(ref fields, _) => fields[0].ty.span, - VariantData::Unit(_) => unreachable!(), - }; - if let Some(snip) = snippet_opt(cx, span) { - db.span_suggestion(span, - "consider boxing the large fields to reduce the total size of \ - the enum", - format!("Box<{}>", snip)); - return; - } - } - db.span_help(def.variants[i].span, - "consider boxing the large fields to reduce the total size of the enum"); - }); - } + + let grouped = (size, (i, variant)); + + update_if(&mut smallest_variant, grouped, |a, b| b.0 <= a.0); + update_if(&mut largest_variant, grouped, |a, b| b.0 >= a.0); }); } + + if let (Some(smallest), Some(largest)) = (smallest_variant, largest_variant) { + let difference = largest.0 - smallest.0; + + if difference > self.maximum_size_difference_allowed { + let (i, variant) = largest.1; + + span_lint_and_then(cx, + LARGE_ENUM_VARIANT, + def.variants[i].span, + "large size difference between variants", + |db| { + if variant.fields.len() == 1 { + let span = match def.variants[i].node.data { + VariantData::Struct(ref fields, _) | + VariantData::Tuple(ref fields, _) => fields[0].ty.span, + VariantData::Unit(_) => unreachable!(), + }; + if let Some(snip) = snippet_opt(cx, span) { + db.span_suggestion(span, + "consider boxing the large fields to reduce the total size of the \ + enum", + format!("Box<{}>", snip)); + return; + } + } + db.span_help(def.variants[i].span, + "consider boxing the large fields to reduce the total size of the enum"); + }); + } + } + } } } + +fn update_if(old: &mut Option, new: T, f: F) + where F: Fn(&T, &T) -> bool +{ + if let Some(ref mut val) = *old { + if f(val, &new) { + *val = new; + } + } else { + *old = Some(new); + } +} diff --git a/tests/ui/large_enum_variant.rs b/tests/ui/large_enum_variant.rs index 5bbcb93910b9..8ac7571c1b1f 100644 --- a/tests/ui/large_enum_variant.rs +++ b/tests/ui/large_enum_variant.rs @@ -8,18 +8,17 @@ enum LargeEnum { A(i32), B([i32; 8000]), - - } -enum GenericEnum { +enum GenericEnumOk { + A(i32), + B([T; 8000]), +} + +enum GenericEnum2 { A(i32), B([i32; 8000]), - - - C([T; 8000]), - D(T, [i32; 8000]), - + C(T, [i32; 8000]), } trait SomeTrait { @@ -27,27 +26,32 @@ trait SomeTrait { } enum LargeEnumGeneric { - Var(A::Item), // regression test, this used to ICE + Var(A::Item), } -enum AnotherLargeEnum { +enum LargeEnum2 { VariantOk(i32, u32), ContainingLargeEnum(LargeEnum), - - +} +enum LargeEnum3 { ContainingMoreThanOneField(i32, [i32; 8000], [i32; 9500]), - VoidVariant, StructLikeLittle { x: i32, y: i32 }, +} + +enum LargeEnum4 { + VariantOk(i32, u32), StructLikeLarge { x: [i32; 8000], y: i32 }, - - StructLikeLarge2 { - x: - [i32; 8000] - - }, } -fn main() { - +enum LargeEnum5 { + VariantOk(i32, u32), + StructLikeLarge2 { x: [i32; 8000] }, } + +enum LargeEnumOk { + LargeA([i32; 8000]), + LargeB([i32; 8001]), +} + +fn main() {} diff --git a/tests/ui/large_enum_variant.stderr b/tests/ui/large_enum_variant.stderr index 77155b6ab5fe..84213003eb70 100644 --- a/tests/ui/large_enum_variant.stderr +++ b/tests/ui/large_enum_variant.stderr @@ -1,4 +1,4 @@ -error: large enum variant found +error: large size difference between variants --> $DIR/large_enum_variant.rs:10:5 | 10 | B([i32; 8000]), @@ -12,73 +12,59 @@ note: lint level defined here help: consider boxing the large fields to reduce the total size of the enum | B(Box<[i32; 8000]>), -error: large enum variant found - --> $DIR/large_enum_variant.rs:17:5 - | -17 | B([i32; 8000]), - | ^^^^^^^^^^^^^^ - | -help: consider boxing the large fields to reduce the total size of the enum - | B(Box<[i32; 8000]>), - -error: large enum variant found +error: large size difference between variants --> $DIR/large_enum_variant.rs:21:5 | -21 | D(T, [i32; 8000]), +21 | C(T, [i32; 8000]), | ^^^^^^^^^^^^^^^^^ | help: consider boxing the large fields to reduce the total size of the enum --> $DIR/large_enum_variant.rs:21:5 | -21 | D(T, [i32; 8000]), +21 | C(T, [i32; 8000]), | ^^^^^^^^^^^^^^^^^ -error: large enum variant found - --> $DIR/large_enum_variant.rs:35:5 +error: large size difference between variants + --> $DIR/large_enum_variant.rs:34:5 | -35 | ContainingLargeEnum(LargeEnum), +34 | ContainingLargeEnum(LargeEnum), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | help: consider boxing the large fields to reduce the total size of the enum | ContainingLargeEnum(Box), -error: large enum variant found - --> $DIR/large_enum_variant.rs:38:5 +error: large size difference between variants + --> $DIR/large_enum_variant.rs:37:5 | -38 | ContainingMoreThanOneField(i32, [i32; 8000], [i32; 9500]), +37 | ContainingMoreThanOneField(i32, [i32; 8000], [i32; 9500]), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | help: consider boxing the large fields to reduce the total size of the enum - --> $DIR/large_enum_variant.rs:38:5 + --> $DIR/large_enum_variant.rs:37:5 | -38 | ContainingMoreThanOneField(i32, [i32; 8000], [i32; 9500]), +37 | ContainingMoreThanOneField(i32, [i32; 8000], [i32; 9500]), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: large enum variant found - --> $DIR/large_enum_variant.rs:42:5 - | -42 | StructLikeLarge { x: [i32; 8000], y: i32 }, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -help: consider boxing the large fields to reduce the total size of the enum - --> $DIR/large_enum_variant.rs:42:5 - | -42 | StructLikeLarge { x: [i32; 8000], y: i32 }, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: large enum variant found +error: large size difference between variants --> $DIR/large_enum_variant.rs:44:5 | -44 | StructLikeLarge2 { - | _____^ starting here... -45 | | x: -46 | | [i32; 8000] -47 | | -48 | | }, - | |_____^ ...ending here +44 | StructLikeLarge { x: [i32; 8000], y: i32 }, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | help: consider boxing the large fields to reduce the total size of the enum - | Box<[i32; 8000]> + --> $DIR/large_enum_variant.rs:44:5 + | +44 | StructLikeLarge { x: [i32; 8000], y: i32 }, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 7 previous errors +error: large size difference between variants + --> $DIR/large_enum_variant.rs:49:5 + | +49 | StructLikeLarge2 { x: [i32; 8000] }, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider boxing the large fields to reduce the total size of the enum + | StructLikeLarge2 { x: Box<[i32; 8000]> }, + +error: aborting due to 6 previous errors