From 89f6012a4db35022ae57cf4cdf2b4ab5fec3feb5 Mon Sep 17 00:00:00 2001
From: Andre Bogus <andre.bogus@aleph-alpha.de>
Date: Thu, 9 Apr 2020 00:05:20 +0200
Subject: [PATCH] compare with the second largest instead of the smallest
 variant

---
 clippy_lints/src/large_enum_variant.rs | 34 ++++++++++----------------
 tests/ui/large_enum_variant.stderr     | 26 +-------------------
 2 files changed, 14 insertions(+), 46 deletions(-)

diff --git a/clippy_lints/src/large_enum_variant.rs b/clippy_lints/src/large_enum_variant.rs
index 961a645a62e97..00bbba64841a9 100644
--- a/clippy_lints/src/large_enum_variant.rs
+++ b/clippy_lints/src/large_enum_variant.rs
@@ -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
@@ -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
@@ -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;
@@ -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);
-    }
-}
diff --git a/tests/ui/large_enum_variant.stderr b/tests/ui/large_enum_variant.stderr
index d3c41ef3c6f30..5d659611533a5 100644
--- a/tests/ui/large_enum_variant.stderr
+++ b/tests/ui/large_enum_variant.stderr
@@ -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
    |
@@ -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
    |
@@ -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