Skip to content

Commit

Permalink
compare with the second largest instead of the smallest variant
Browse files Browse the repository at this point in the history
  • Loading branch information
Andre Bogus committed Apr 10, 2020
1 parent d342cee commit 89f6012
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 46 deletions.
34 changes: 13 additions & 21 deletions clippy_lints/src/large_enum_variant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,13 @@ declare_clippy_lint! {
/// `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.
/// large variant can penalize the memory layout of that enum.
///
/// **Known problems:** None.
/// **Known problems:** This lint obviously cannot take the distribution of
/// variants in your running program into account. It is possible that the
/// smaller variants make up less than 1% of all instances, in which case
/// the overhead is negligible and the boxing is counter-productive. Always
/// measure the change this lint suggests.
///
/// **Example:**
/// ```rust
Expand Down Expand Up @@ -52,8 +55,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LargeEnumVariant {
let ty = cx.tcx.type_of(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;
let mut second_variant: Option<(_, _)> = None;

for (i, variant) in adt.variants.iter().enumerate() {
let size: u64 = variant
Expand All @@ -69,12 +72,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LargeEnumVariant {

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 grouped.0 >= largest_variant.map_or(0, |x| x.0) {
second_variant = largest_variant;
largest_variant = Some(grouped);
}
}

if let (Some(smallest), Some(largest)) = (smallest_variant, largest_variant) {
let difference = largest.0 - smallest.0;
if let (Some(largest), Some(second)) = (largest_variant, second_variant) {
let difference = largest.0 - second.0;

if difference > self.maximum_size_difference_allowed {
let (i, variant) = largest.1;
Expand Down Expand Up @@ -114,16 +119,3 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LargeEnumVariant {
}
}
}

fn update_if<T, F>(old: &mut Option<T>, 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);
}
}
26 changes: 1 addition & 25 deletions tests/ui/large_enum_variant.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,6 @@ help: consider boxing the large fields to reduce the total size of the enum
LL | B(Box<[i32; 8000]>),
| ^^^^^^^^^^^^^^^^

error: large size difference between variants
--> $DIR/large_enum_variant.rs:18:5
|
LL | C(T, [i32; 8000]),
| ^^^^^^^^^^^^^^^^^
|
help: consider boxing the large fields to reduce the total size of the enum
--> $DIR/large_enum_variant.rs:18:5
|
LL | C(T, [i32; 8000]),
| ^^^^^^^^^^^^^^^^^

error: large size difference between variants
--> $DIR/large_enum_variant.rs:31:5
|
Expand All @@ -33,18 +21,6 @@ help: consider boxing the large fields to reduce the total size of the enum
LL | ContainingLargeEnum(Box<LargeEnum>),
| ^^^^^^^^^^^^^^

error: large size difference between variants
--> $DIR/large_enum_variant.rs:34:5
|
LL | 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:34:5
|
LL | ContainingMoreThanOneField(i32, [i32; 8000], [i32; 9500]),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: large size difference between variants
--> $DIR/large_enum_variant.rs:41:5
|
Expand All @@ -68,5 +44,5 @@ help: consider boxing the large fields to reduce the total size of the enum
LL | StructLikeLarge2 { x: Box<[i32; 8000]> },
| ^^^^^^^^^^^^^^^^

error: aborting due to 6 previous errors
error: aborting due to 4 previous errors

0 comments on commit 89f6012

Please sign in to comment.