Apply suggestions from code review

tracing-subscriber/src/reload.rs

@@ -148,10 +148,16 @@ where
 
     #[doc(hidden)]
     unsafe fn downcast_raw(&self, id: TypeId) -> Option<NonNull<()>> {
-        // Safety: It is generally unsafe to downcast through a reload, because
+        // Safety: it is generally unsafe to downcast through a reload, because
         // the pointer can be invalidated after the lock is dropped.
         // `NoneLayerMarker` is a special case because it
         // is never dereferenced.
+        //
+        // Additionally, even if the marker type *is* dereferenced (which it
+        // never will be), the pointer should be valid even if the subscriber
+        // is reloaded, because all `NoneLayerMarker` pointers that we return
+        // actually point to the global static singleton `NoneLayerMarker`,
+        // rather than to a field inside the lock.
         if id == TypeId::of::<subscribe::NoneLayerMarker>() {
             return try_lock!(self.inner.read(), else return None).downcast_raw(id);
         }

tracing-subscriber/src/subscribe/mod.rs

@@ -1493,8 +1493,8 @@ pub struct Identity {
 // === impl Subscribe ===
 
 #[derive(Clone, Copy)]
-pub(crate) struct NoneLayerMarker;
-pub(crate) static NONE_LAYER_MARKER: NoneLayerMarker = NoneLayerMarker;
+pub(crate) struct NoneLayerMarker(());
+pub(crate) static NONE_LAYER_MARKER: NoneLayerMarker = NoneLayerMarker(());
 
 /// Is a type implementing `Subscriber` `Option::<_>::None`?
 pub(crate) fn subscriber_is_none<S, C>(subscriber: &S) -> bool
@@ -1503,10 +1503,13 @@ where
     C: Collect,
 {
     unsafe {
-        // Safety: we're not actually *doing* anything with this pointer --- we
-        // only care about the `Option`, which we're turning into a `bool`. So
-        // even if the subscriber decides to be evil and give us some kind of invalid
-        // pointer, we don't ever dereference it, so this is always safe.
+        // Safety: we're not actually *doing* anything with this pointer ---
+        // this only care about the `Option`, which is essentially being used
+        // as a bool. We can rely on the pointer being valid, because it is
+        // a crate-private type, and is only returned by the `Subscribe` impl
+        // for `Option`s. However, even if the subscriber *does* decide to be
+        // evil and give us an invalid pointer here, that's fine, because we'll
+        // never actually dereference it.
         subscriber.downcast_raw(TypeId::of::<NoneLayerMarker>())
     }
     .is_some()