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.

Sign up for GitHub

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

compare with the second largest instead of the smallest variant #5446

Merged
merged 1 commit into from
Apr 10, 2020
Merged

compare with the second largest instead of the smallest variant #5446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 13 additions & 21 deletions clippy_lints/src/large_enum_variant.rs
View file
Open in desktop
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
View file
Open in desktop
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