Review feedback

Author: luqmana

compiler/rustc_abi/src/layout.rs

@@ -729,34 +729,43 @@ pub trait LayoutCalculator {
             align = align.max(AbiAndPrefAlign::new(repr_align));
         }
 
-        let mut optimize = !repr.inhibit_union_abi_opt();
+        // If all the non-ZST fields have the same ABI and union ABI optimizations aren't
+        // disabled, we can use that common ABI for the union as a whole.
+        struct AbiMismatch;
+        let mut common_non_zst_abi_and_align = if repr.inhibit_union_abi_opt() {
+            // Can't optimize
+            Err(AbiMismatch)
+        } else {
+            Ok(None)
+        };
+
         let mut size = Size::ZERO;
-        let mut common_non_zst_abi_and_align: Option<(Abi, AbiAndPrefAlign)> = None;
         let only_variant = &variants[FIRST_VARIANT];
         for field in only_variant {
             assert!(field.0.is_sized());
 
-            if !field.0.is_zst() && optimize {
+            if !field.0.is_zst() && !common_non_zst_abi_and_align.is_err() {
                 // Discard valid range information and allow undef
                 let field_abi = field.abi().to_union();
 
-                if let Some((abi, align)) = &mut common_non_zst_abi_and_align {
-                    if *abi != field_abi {
+                if let Ok(Some((common_abi, common_align))) = &mut common_non_zst_abi_and_align {
+                    if *common_abi != field_abi {
                         // Different fields have different ABI: disable opt
-                        optimize = false;
+                        common_non_zst_abi_and_align = Err(AbiMismatch);
                     } else {
                         // Fields with the same non-Aggregate ABI should also
                         // have the same alignment
-                        if !matches!(abi, Abi::Aggregate { .. }) {
+                        if !matches!(common_abi, Abi::Aggregate { .. }) {
                             assert_eq!(
-                                align.abi,
+                                *common_align,
                                 field.align().abi,
                                 "non-Aggregate field with matching ABI but differing alignment"
                             );
                         }
                     }
                 } else {
-                    common_non_zst_abi_and_align = Some((field_abi, field.align()));
+                    // First non-ZST field: record its ABI and alignment
+                    common_non_zst_abi_and_align = Ok(Some((field_abi, field.align().abi)));
                 }
             }
 
@@ -770,11 +779,11 @@ pub trait LayoutCalculator {
 
         // If all non-ZST fields have the same ABI, we may forward that ABI
         // for the union as a whole, unless otherwise inhibited.
-        let abi = match (optimize, common_non_zst_abi_and_align) {
-            (false, _) | (_, None) => Abi::Aggregate { sized: true },
-            (true, Some((abi, _))) => {
+        let abi = match common_non_zst_abi_and_align {
+            Err(AbiMismatch) | Ok(None) => Abi::Aggregate { sized: true },
+            Ok(Some((abi, _))) => {
                 if abi.inherent_align(dl).map(|a| a.abi) != Some(align.abi) {
-                    // Mismatched alignment: disable opt
+                    // Mismatched alignment (e.g. union is #[repr(packed)]): disable opt
                     Abi::Aggregate { sized: true }
                 } else {
                     abi

compiler/rustc_abi/src/lib.rs

@@ -1306,7 +1306,7 @@ impl Abi {
         })
     }
 
-    /// Discard valid range information and allow undef
+    /// Discard validity range information and allow undef.
     pub fn to_union(&self) -> Self {
         assert!(self.is_sized());
         match *self {