From 6b068437cb47dd9ca783171525ae891f383190ae Mon Sep 17 00:00:00 2001
From: Thom Chiovoloni <chiovolonit@gmail.com>
Date: Mon, 9 Aug 2021 02:19:28 -0700
Subject: [PATCH] Address address comments, improve comments slightly

---
 library/std/src/io/error.rs                |  7 +++++++
 library/std/src/io/error/repr_bitpacked.rs | 18 +++++++++++++-----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/library/std/src/io/error.rs b/library/std/src/io/error.rs
index 449771c6761..8f82778e90b 100644
--- a/library/std/src/io/error.rs
+++ b/library/std/src/io/error.rs
@@ -83,6 +83,10 @@ enum ErrorData<C> {
     Custom(C),
 }
 
+// `#[repr(align(4))]` is probably redundant, it should have that value or
+// higher already. We include it just because repr_bitpacked.rs's encoding
+// requires an alignment >= 4 (note that `#[repr(align)]` will not reduce the
+// alignment required by the struct, only increase it).
 #[repr(align(4))]
 #[doc(hidden)]
 pub(crate) struct SimpleMessage {
@@ -106,6 +110,9 @@ pub(crate) macro const_io_error($kind:expr, $message:expr $(,)?) {
     })
 }
 
+// As with `SimpleMessage`: `#[repr(align(4))]` here is just because
+// repr_bitpacked's encoding requires it. In practice it almost certainly be
+// already be this high or higher.
 #[derive(Debug)]
 #[repr(align(4))]
 struct Custom {
diff --git a/library/std/src/io/error/repr_bitpacked.rs b/library/std/src/io/error/repr_bitpacked.rs
index 8ff0bcc7cb5..281cbdc23e4 100644
--- a/library/std/src/io/error/repr_bitpacked.rs
+++ b/library/std/src/io/error/repr_bitpacked.rs
@@ -56,7 +56,7 @@
 //!
 //! Conceptually you might think of this more like:
 //!
-//! ```ignore
+//! ```ignore (exposition-only)
 //! union Repr {
 //!     // holds integer (Simple/Os) variants, and
 //!     // provides access to the tag bits.
@@ -159,7 +159,7 @@ impl Repr {
 
     #[inline]
     pub(super) const fn new_simple_message(m: &'static SimpleMessage) -> Self {
-        // Safety: We're a Repr, decode_repr is fine.
+        // Safety: References are never null.
         Self(unsafe { NonNull::new_unchecked(m as *const _ as *mut ()) })
     }
 
@@ -213,7 +213,7 @@ where
         TAG_SIMPLE => {
             let kind_bits = (bits >> 32) as u32;
             let kind = kind_from_prim(kind_bits).unwrap_or_else(|| {
-                debug_assert!(false, "Invalid io::error::Repr bits: `Repr({:#016x})`", bits);
+                debug_assert!(false, "Invalid io::error::Repr bits: `Repr({:#018x})`", bits);
                 // This means the `ptr` passed in was not valid, which voilates
                 // the unsafe contract of `decode_repr`.
                 //
@@ -299,8 +299,11 @@ fn kind_from_prim(ek: u32) -> Option<ErrorKind> {
 }
 
 // Some static checking to alert us if a change breaks any of the assumptions
-// that our encoding relies on. If any of these are hit on a platform that
-// libstd supports, we should just make sure `repr_unpacked.rs` is used.
+// that our encoding relies on for correctness and soundness. (Some of these are
+// a bit overly thorough/cautious, admittedly)
+//
+// If any of these are hit on a platform that libstd supports, we should just
+// make sure `repr_unpacked.rs` is used instead.
 macro_rules! static_assert {
     ($condition:expr) => {
         const _: [(); 0] = [(); (!$condition) as usize];
@@ -332,6 +335,11 @@ static_assert!(TAG_SIMPLE != 0);
 static_assert!(TAG_SIMPLE_MESSAGE == 0);
 
 // Check that the point of all of this still holds.
+//
+// We'd check against `io::Error`, but *technically* it's allowed to vary,
+// as it's not `#[repr(transparent)]`/`#[repr(C)]`. We could add that, but
+// the `#[repr()]` would show up in rustdoc, which might be seen as a stable
+// commitment.
 static_assert!(size_of::<Repr>() == 8);
 static_assert!(size_of::<Option<Repr>>() == 8);
 static_assert!(size_of::<Result<(), Repr>>() == 8);