Auto merge of #67246 - JohnTitor:rollup-nfa7skn, r=JohnTitor

Rollup of 8 pull requests

Successful merges:

 - #62514 (Clarify `Box<T>` representation and its use in FFI)
 - #66983 (Fix `unused_parens` triggers on macro by example code)
 - #67215 (Fix `-Z print-type-sizes`'s handling of zero-sized fields.)
 - #67230 (Remove irelevant comment on `register_dtor`)
 - #67236 (resolve: Always resolve visibilities on impl items)
 - #67237 (Some small readability improvements)
 - #67238 (Small std::borrow::Cow improvements)
 - #67239 (Make TinyList::remove iterate instead of recurse)

Failed merges:

r? @ghost

src/liballoc/borrow.rs

@@ -195,14 +195,10 @@ impl<B: ?Sized + ToOwned> Clone for Cow<'_, B> {
     }
 
     fn clone_from(&mut self, source: &Self) {
-        if let Owned(ref mut dest) = *self {
-            if let Owned(ref o) = *source {
-                o.borrow().clone_into(dest);
-                return;
-            }
+        match (self, source) {
+            (&mut Owned(ref mut dest), &Owned(ref o)) => o.borrow().clone_into(dest),
+            (t, s) => *t = s.clone(),
         }
-
-        *self = source.clone();
     }
 }
 
@@ -449,9 +445,7 @@ impl<'a> AddAssign<&'a str> for Cow<'a, str> {
     fn add_assign(&mut self, rhs: &'a str) {
         if self.is_empty() {
             *self = Cow::Borrowed(rhs)
-        } else if rhs.is_empty() {
-            return;
-        } else {
+        } else if !rhs.is_empty() {
             if let Cow::Borrowed(lhs) = *self {
                 let mut s = String::with_capacity(lhs.len() + rhs.len());
                 s.push_str(lhs);
@@ -467,9 +461,7 @@ impl<'a> AddAssign<Cow<'a, str>> for Cow<'a, str> {
     fn add_assign(&mut self, rhs: Cow<'a, str>) {
         if self.is_empty() {
             *self = rhs
-        } else if rhs.is_empty() {
-            return;
-        } else {
+        } else if !rhs.is_empty() {
             if let Cow::Borrowed(lhs) = *self {
                 let mut s = String::with_capacity(lhs.len() + rhs.len());
                 s.push_str(lhs);

src/liballoc/boxed.rs

@@ -61,7 +61,60 @@
 //! T` obtained from [`Box::<T>::into_raw`] may be deallocated using the
 //! [`Global`] allocator with [`Layout::for_value(&*value)`].
 //!
+//! So long as `T: Sized`, a `Box<T>` is guaranteed to be represented
+//! as a single pointer and is also ABI-compatible with C pointers
+//! (i.e. the C type `T*`). This means that if you have extern "C"
+//! Rust functions that will be called from C, you can define those
+//! Rust functions using `Box<T>` types, and use `T*` as corresponding
+//! type on the C side. As an example, consider this C header which
+//! declares functions that create and destroy some kind of `Foo`
+//! value:
 //!
+//! ```c
+//! /* C header */
+//!
+//! /* Returns ownership to the caller */
+//! struct Foo* foo_new(void);
+//!
+//! /* Takes ownership from the caller; no-op when invoked with NULL */
+//! void foo_delete(struct Foo*);
+//! ```
+//!
+//! These two functions might be implemented in Rust as follows. Here, the
+//! `struct Foo*` type from C is translated to `Box<Foo>`, which captures
+//! the ownership constraints. Note also that the nullable argument to
+//! `foo_delete` is represented in Rust as `Option<Box<Foo>>`, since `Box<Foo>`
+//! cannot be null.
+//!
+//! ```
+//! #[repr(C)]
+//! pub struct Foo;
+//!
+//! #[no_mangle]
+//! pub extern "C" fn foo_new() -> Box<Foo> {
+//!     Box::new(Foo)
+//! }
+//!
+//! #[no_mangle]
+//! pub extern "C" fn foo_delete(_: Option<Box<Foo>>) {}
+//! ```
+//!
+//! Even though `Box<T>` has the same representation and C ABI as a C pointer,
+//! this does not mean that you can convert an arbitrary `T*` into a `Box<T>`
+//! and expect things to work. `Box<T>` values will always be fully aligned,
+//! non-null pointers. Moreover, the destructor for `Box<T>` will attempt to
+//! free the value with the global allocator. In general, the best practice
+//! is to only use `Box<T>` for pointers that originated from the global
+//! allocator.
+//!
+//! **Important.** At least at present, you should avoid using
+//! `Box<T>` types for functions that are defined in C but invoked
+//! from Rust. In those cases, you should directly mirror the C types
+//! as closely as possible. Using types like `Box<T>` where the C
+//! definition is just using `T*` can lead to undefined behavior, as
+//! described in [rust-lang/unsafe-code-guidelines#198][ucg#198].
+//!
+//! [ucg#198]: https://github.com/rust-lang/unsafe-code-guidelines/issues/198
 //! [dereferencing]: ../../std/ops/trait.Deref.html
 //! [`Box`]: struct.Box.html
 //! [`Box<T>`]: struct.Box.html

src/liballoc/tests/cow_str.rs

@@ -138,4 +138,7 @@ fn check_cow_clone_from() {
     let c2: Cow<'_, str> = Cow::Owned(s);
     c1.clone_from(&c2);
     assert!(c1.into_owned().capacity() >= 25);
+    let mut c3: Cow<'_, str> = Cow::Borrowed("bye");
+    c3.clone_from(&c2);
+    assert_eq!(c2, c3);
 }

src/libcore/char/methods.rs

@@ -553,8 +553,7 @@ impl char {
     pub fn is_alphabetic(self) -> bool {
         match self {
             'a'..='z' | 'A'..='Z' => true,
-            c if c > '\x7f' => derived_property::Alphabetic(c),
-            _ => false,
+            c => c > '\x7f' && derived_property::Alphabetic(c),
         }
     }
 
@@ -585,8 +584,7 @@ impl char {
     pub fn is_lowercase(self) -> bool {
         match self {
             'a'..='z' => true,
-            c if c > '\x7f' => derived_property::Lowercase(c),
-            _ => false,
+            c => c > '\x7f' && derived_property::Lowercase(c),
         }
     }
 
@@ -617,8 +615,7 @@ impl char {
     pub fn is_uppercase(self) -> bool {
         match self {
             'A'..='Z' => true,
-            c if c > '\x7f' => derived_property::Uppercase(c),
-            _ => false,
+            c => c > '\x7f' && derived_property::Uppercase(c),
         }
     }
 
@@ -646,8 +643,7 @@ impl char {
     pub fn is_whitespace(self) -> bool {
         match self {
             ' ' | '\x09'..='\x0d' => true,
-            c if c > '\x7f' => property::White_Space(c),
-            _ => false,
+            c => c > '\x7f' && property::White_Space(c),
         }
     }
 
@@ -744,8 +740,7 @@ impl char {
     pub fn is_numeric(self) -> bool {
         match self {
             '0'..='9' => true,
-            c if c > '\x7f' => general_category::N(c),
-            _ => false,
+            c => c > '\x7f' && general_category::N(c),
         }
     }
 

src/libcore/str/pattern.rs

@@ -296,12 +296,7 @@ unsafe impl<'a> Searcher<'a> for CharSearcher<'a> {
     fn next_match(&mut self) -> Option<(usize, usize)> {
         loop {
             // get the haystack after the last character found
-            let bytes = if let Some(slice) = self.haystack.as_bytes()
-                                                 .get(self.finger..self.finger_back) {
-                slice
-            } else {
-                return None;
-            };
+            let bytes = self.haystack.as_bytes().get(self.finger..self.finger_back)?;
             // the last byte of the utf8 encoded needle
             let last_byte = unsafe { *self.utf8_encoded.get_unchecked(self.utf8_size - 1) };
             if let Some(index) = memchr::memchr(last_byte, bytes) {

src/librustc_data_structures/tiny_list.rs

@@ -16,41 +16,29 @@ mod tests;
 
 #[derive(Clone)]
 pub struct TinyList<T: PartialEq> {
-    head: Option<Element<T>>
+    head: Option<Element<T>>,
 }
 
 impl<T: PartialEq> TinyList<T> {
     #[inline]
     pub fn new() -> TinyList<T> {
-        TinyList {
-            head: None
-        }
+        TinyList { head: None }
     }
 
     #[inline]
     pub fn new_single(data: T) -> TinyList<T> {
-        TinyList {
-            head: Some(Element {
-                data,
-                next: None,
-            })
-        }
+        TinyList { head: Some(Element { data, next: None }) }
     }
 
     #[inline]
     pub fn insert(&mut self, data: T) {
-        self.head = Some(Element {
-            data,
-            next: self.head.take().map(Box::new)
-        });
+        self.head = Some(Element { data, next: self.head.take().map(Box::new) });
     }
 
     #[inline]
     pub fn remove(&mut self, data: &T) -> bool {
         self.head = match self.head {
-            Some(ref mut head) if head.data == *data => {
-                head.next.take().map(|x| *x)
-            }
+            Some(ref mut head) if head.data == *data => head.next.take().map(|x| *x),
             Some(ref mut head) => return head.remove_next(data),
             None => return false,
         };
@@ -88,12 +76,16 @@ struct Element<T: PartialEq> {
 
 impl<T: PartialEq> Element<T> {
     fn remove_next(&mut self, data: &T) -> bool {
-        let new_next = match self.next {
-            Some(ref mut next) if next.data == *data => next.next.take(),
-            Some(ref mut next) => return next.remove_next(data),
-            None => return false,
-        };
-        self.next = new_next;
-        true
+        let mut n = self;
+        loop {
+            match n.next {
+                Some(ref mut next) if next.data == *data => {
+                    n.next = next.next.take();
+                    return true;
+                }
+                Some(ref mut next) => n = next,
+                None => return false,
+            }
+        }
     }
 }

src/librustc_lint/unused.rs

@@ -355,7 +355,9 @@ impl UnusedParens {
         match value.kind {
             ast::ExprKind::Paren(ref inner) => {
                 if !Self::is_expr_parens_necessary(inner, followed_by_block) &&
-                    value.attrs.is_empty() {
+                    value.attrs.is_empty() &&
+                    !value.span.from_expansion()
+                {
                     let expr_text = if let Ok(snippet) = cx.sess().source_map()
                         .span_to_snippet(value.span) {
                             snippet

src/librustc_resolve/build_reduced_graph.rs

@@ -647,8 +647,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
                 self.r.define(parent, ident, TypeNS, imported_binding);
             }
 
-            ItemKind::GlobalAsm(..) => {}
-
             ItemKind::Mod(..) if ident.name == kw::Invalid => {} // Crate root
 
             ItemKind::Mod(..) => {
@@ -667,9 +665,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
                 self.parent_scope.module = module;
             }
 
-            // Handled in `rustc_metadata::{native_libs,link_args}`
-            ItemKind::ForeignMod(..) => {}
-
             // These items live in the value namespace.
             ItemKind::Static(..) => {
                 let res = Res::Def(DefKind::Static, self.r.definitions.local_def_id(item.id));
@@ -765,12 +760,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
                 self.insert_field_names_local(def_id, vdata);
             }
 
-            ItemKind::Impl(.., ref impl_items) => {
-                for impl_item in impl_items {
-                    self.resolve_visibility(&impl_item.vis);
-                }
-            }
-
             ItemKind::Trait(..) => {
                 let def_id = self.r.definitions.local_def_id(item.id);
 
@@ -785,6 +774,9 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
                 self.parent_scope.module = module;
             }
 
+            // These items do not add names to modules.
+            ItemKind::Impl(..) | ItemKind::ForeignMod(..) | ItemKind::GlobalAsm(..) => {}
+
             ItemKind::MacroDef(..) | ItemKind::Mac(_) => unreachable!(),
         }
     }
@@ -1118,7 +1110,6 @@ macro_rules! method {
 }
 
 impl<'a, 'b> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b> {
-    method!(visit_impl_item: ast::ImplItem, ast::ImplItemKind::Macro, walk_impl_item);
     method!(visit_expr:      ast::Expr,     ast::ExprKind::Mac,       walk_expr);
     method!(visit_pat:       ast::Pat,      ast::PatKind::Mac,        walk_pat);
     method!(visit_ty:        ast::Ty,       ast::TyKind::Mac,         walk_ty);
@@ -1202,6 +1193,15 @@ impl<'a, 'b> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b> {
         visit::walk_trait_item(self, item);
     }
 
+    fn visit_impl_item(&mut self, item: &'b ast::ImplItem) {
+        if let ast::ImplItemKind::Macro(..) = item.kind {
+            self.visit_invoc(item.id);
+        } else {
+            self.resolve_visibility(&item.vis);
+            visit::walk_impl_item(self, item);
+        }
+    }
+
     fn visit_token(&mut self, t: Token) {
         if let token::Interpolated(nt) = t.kind {
             if let token::NtExpr(ref expr) = *nt {

src/librustc_session/code_stats.rs

@@ -132,9 +132,12 @@ impl CodeStats {
 
                 let mut min_offset = discr_size;
 
-                // We want to print fields by increasing offset.
+                // We want to print fields by increasing offset. We also want
+                // zero-sized fields before non-zero-sized fields, otherwise
+                // the loop below goes wrong; hence the `f.size` in the sort
+                // key.
                 let mut fields = fields.clone();
-                fields.sort_by_key(|f| f.offset);
+                fields.sort_by_key(|f| (f.offset, f.size));
 
                 for field in fields.iter() {
                     let FieldInfo { ref name, offset, size, align } = *field;
@@ -146,7 +149,7 @@ impl CodeStats {
                     }
 
                     if offset < min_offset {
-                        // if this happens something is very wrong
+                        // If this happens it's probably a union.
                         println!("print-type-size {}field `.{}`: {} bytes, \
                                   offset: {} bytes, \
                                   alignment: {} bytes",

src/libstd/sys/unix/fast_thread_local.rs

@@ -8,8 +8,6 @@
 // Note, however, that we run on lots older linuxes, as well as cross
 // compiling from a newer linux to an older linux, so we also have a
 // fallback implementation to use as well.
-//
-// Due to rust-lang/rust#18804, make sure this is not generic!
 #[cfg(any(
     target_os = "linux",
     target_os = "fuchsia",

src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.rs

@@ -17,10 +17,7 @@ macro_rules! the_worship_the_heart_lifts_above {
 
 macro_rules! and_the_heavens_reject_not {
     () => {
-        // ↓ But let's test that we still lint for unused parens around
-        // function args inside of simple, one-deep macros.
         #[allow(dead_code)] fn the_night_for_the_morrow() -> Option<isize> { Some((2)) }
-        //~^ WARN unnecessary parentheses around function argument
     }
 }