Bug 1316696 - Eliminate ns{Fixed,}{C,}StringRepr from the rust bindings, r=froydnj
authorMichael Layzell <michael@thelayzells.com>
Fri, 11 Nov 2016 15:26:01 -0500
changeset 324234 b368ec57dc57d5fc4edd00c4c505fe52d10ad160
parent 324233 9ff1755fa83c99676b53f3858c1ab9375837396b
child 324235 3a36e60d1c4ce25062d843d6cabd970c4c9cc84d
push id30993
push usercbook@mozilla.com
push dateFri, 25 Nov 2016 14:40:38 +0000
treeherdermozilla-central@b982373cb0e9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1316696
milestone53.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1316696 - Eliminate ns{Fixed,}{C,}StringRepr from the rust bindings, r=froydnj MozReview-Commit-ID: AMwAUq82vMo
xpcom/rust/nsstring/src/lib.rs
--- a/xpcom/rust/nsstring/src/lib.rs
+++ b/xpcom/rust/nsstring/src/lib.rs
@@ -1,31 +1,28 @@
 //! This module provides rust bindings for the XPCOM string types.
 //!
 //! # TL;DR (what types should I use)
 //!
 //! Use `&{mut,} nsA[C]String` for functions in rust which wish to take or
 //! mutate XPCOM strings. The other string types `Deref` to this type.
 //!
-//! Use `ns[C]String<'a>` for string struct members which don't leave rust, and
+//! Use `ns[C]String<'a>` (`ns[C]String` in C++) for string struct members, and
 //! as an intermediate between rust string data structures (such as `String`,
 //! `Vec<u16>`, `&str`, and `&[u16]`) and `&{mut,} nsA[C]String` (using
 //! `ns[C]String::from(value)`). These conversions, when possible, will not
-//! perform any allocations.
+//! perform any allocations. When using this type in structs shared with C++,
+//! the correct lifetime argument is usually `'static`.
 //!
 //! Use `nsFixed[C]String` or `ns_auto_[c]string!` for dynamic stack allocated
 //! strings which are expected to hold short string values.
 //!
 //! Use `*{const,mut} nsA[C]String` (`{const,} nsA[C]String*` in C++) for
 //! function arguments passed across the rust/C++ language boundary.
 //!
-//! Use `ns[C]StringRepr` for string struct members which are shared between
-//! rust and C++, but be careful, because this type lacks a `Drop`
-//! implementation.
-//!
 //! # String Types
 //!
 //! ## `nsA[C]String`
 //!
 //! The core types in this module are `nsAString` and `nsACString`. These types
 //! are zero-sized as far as rust is concerned, and are safe to pass around
 //! behind both references (in rust code), and pointers (in C++ code). They
 //! represent a handle to a XPCOM string which holds either `u16` or `u8`
@@ -65,78 +62,53 @@
 //! `From` implementations. Both string types may be constructed `From<&'a
 //! str>`, with `nsCString` having a `'a` lifetime, as the storage is shared
 //! with the `str`, while `nsString` has a `'static` lifetime, as its storage
 //! has to be transcoded.
 //!
 //! When passing this type by reference, prefer passing a `&nsA[C]String` or
 //! `&mut nsA[C]String`. to passing this type.
 //!
-//! This type is _not_ `#[repr(C)]`, as it has a `Drop` impl, which in versions
-//! of `rustc < 1.13` adds drop flags to the struct, which messes up the layout,
-//! making it unsafe to pass across the FFI boundary. The rust compiler will
-//! warn if this type appears in `extern "C"` function definitions.
-//!
 //! When passing this type across the language boundary, pass it as `*const
 //! nsA[C]String` for an immutable reference, or `*mut nsA[C]String` for a
-//! mutable reference.
-//!
-//! This type is similar to the C++ type of the same name.
+//! mutable reference. This struct may also be included in `#[repr(C)]`
+//! structs shared with C++.
 //!
 //! ## `nsFixed[C]String<'a>`
 //!
 //! This type is a string type with fixed backing storage. It is created with
 //! `nsFixed[C]String::new(buffer)`, passing a mutable reference to a buffer as
 //! the argument. This buffer will be used as backing storage whenever the
 //! resulting string will fit within it, falling back to heap allocations only
 //! when the string size exceeds that of the backing buffer.
 //!
 //! Like `ns[C]String`, this type dereferences to `nsA[C]String` which provides
 //! the methods for manipulating the type, and is not `#[repr(C)]`.
 //!
 //! When passing this type by reference, prefer passing a `&nsA[C]String` or
 //! `&mut nsA[C]String`. to passing this type.
 //!
-//! This type is _not_ `#[repr(C)]`, as it has a `Drop` impl, which in versions
-//! of `rustc < 1.13` adds drop flags to the struct, which messes up the layout,
-//! making it unsafe to pass across the FFI boundary. The rust compiler will
-//! warn if this type appears in `extern "C"` function definitions.
-//!
 //! When passing this type across the language boundary, pass it as `*const
 //! nsA[C]String` for an immutable reference, or `*mut nsA[C]String` for a
-//! mutable reference.
-//!
-//! This type is similar to the C++ type of the same name.
+//! mutable reference. This struct may also be included in `#[repr(C)]`
+//! structs shared with C++, although `nsFixed[C]String` objects are uncommon
+//! as struct members.
 //!
 //! ## `ns_auto_[c]string!($name)`
 //!
 //! This is a helper macro which defines a fixed size, (currently 64 character),
 //! backing array on the stack, and defines a local variable with name `$name`
 //! which is a `nsFixed[C]String` using this buffer as its backing storage.
 //!
 //! Usage of this macro is similar to the C++ type `nsAuto[C]String`, but could
 //! not be implemented as a basic type due to the differences between rust and
 //! C++'s move semantics.
-//!
-//! ## `ns[C]StringRepr`
-//!
-//! This type represents a C++ `ns[C]String`. This type is `#[repr(C)]` and is
-//! safe to use in struct definitions which are shared across the language
-//! boundary. It automatically dereferences to `&{mut,} nsA[C]String`, and thus
-//! can be treated similarially to `ns[C]String`.
-//!
-//! If this type is dropped in rust, it will not free its backing storage. This
-//! is because types implementing `Drop` have a drop flag added, which messes up
-//! the layout of this type. When drop flags are removed, which should happen in
-//! `rustc 1.13` (see rust-lang/rust#35764), this type will likely be removed,
-//! and replaced with direct usage of `ns[C]String<'a>`, as its layout may be
-//! identical. This module provides rust bindings to our xpcom ns[C]String
-//! types.
 
 #![allow(non_camel_case_types)]
+#![deny(warnings)]
 
 use std::ops::{Deref, DerefMut};
 use std::marker::PhantomData;
 use std::slice;
 use std::ptr;
 use std::mem;
 use std::fmt;
 use std::cmp;
@@ -165,68 +137,17 @@ const F_CLASS_FIXED: u32 = 1 << 16; // i
 ////////////////////////////////////
 
 macro_rules! define_string_types {
     {
         char_t = $char_t: ty;
         AString = $AString: ident;
         String = $String: ident;
         FixedString = $FixedString: ident;
-
-        StringRepr = $StringRepr: ident;
-        FixedStringRepr = $FixedStringRepr: ident;
-        AutoStringRepr = $AutoStringRepr: ident;
     } => {
-        /// The representation of a ns[C]String type in C++. This type is
-        /// used internally by our definition of ns[C]String to ensure layout
-        /// compatibility with the C++ ns[C]String type.
-        ///
-        /// This type may also be used in place of a C++ ns[C]String inside of
-        /// struct definitions which are shared with C++, as it has identical
-        /// layout to our ns[C]String type. Due to drop flags, our ns[C]String
-        /// type does not have identical layout. When drop flags are removed,
-        /// this type will likely be made a private implementation detail, and
-        /// its uses will be replaced with `ns[C]String`.
-        ///
-        /// This struct will leak its data if dropped from rust. See the module
-        /// documentation for more information on this type.
-        #[repr(C)]
-        pub struct $StringRepr {
-            data: *const $char_t,
-            length: u32,
-            flags: u32,
-        }
-
-        impl Deref for $StringRepr {
-            type Target = $AString;
-            fn deref(&self) -> &$AString {
-                unsafe {
-                    mem::transmute(self)
-                }
-            }
-        }
-
-        impl DerefMut for $StringRepr {
-            fn deref_mut(&mut self) -> &mut $AString {
-                unsafe {
-                    mem::transmute(self)
-                }
-            }
-        }
-
-        /// The representation of a nsFixed[C]String type in C++. This type is
-        /// used internally by our definition of nsFixed[C]String to ensure layout
-        /// compatibility with the C++ nsFixed[C]String type.
-        #[repr(C)]
-        struct $FixedStringRepr {
-            base: $StringRepr,
-            capacity: u32,
-            buffer: *mut $char_t,
-        }
-
         /// This type is the abstract type which is used for interacting with
         /// strings in rust. Each string type can derefence to an instance of
         /// this type, which provides the useful operations on strings.
         ///
         /// NOTE: Rust thinks this type has a size of 0, because the data
         /// associated with it is not necessarially safe to move. It is not safe
         /// to construct a nsAString yourself, unless it is received by
         /// dereferencing one of these types.
@@ -239,19 +160,20 @@ macro_rules! define_string_types {
         pub struct $AString {
             _prohibit_constructor: [u8; 0],
         }
 
         impl Deref for $AString {
             type Target = [$char_t];
             fn deref(&self) -> &[$char_t] {
                 unsafe {
-                    // This is legal, as all $AString values actually point to a
-                    // $StringRepr
-                    let this: &$StringRepr = mem::transmute(self);
+                    // All $AString values point to a struct prefix which is identical
+                    // to $String, thus we can transmute `self` into $String to get
+                    // the reference to the underlying data.
+                    let this: &$String = mem::transmute(self);
                     if this.data.is_null() {
                         debug_assert!(this.length == 0);
                         // Use an arbitrary non-null value as the pointer
                         slice::from_raw_parts(0x1 as *const $char_t, 0)
                     } else {
                         slice::from_raw_parts(this.data, this.length as usize)
                     }
                 }
@@ -283,44 +205,49 @@ macro_rules! define_string_types {
         }
 
         impl<'a> cmp::PartialEq<$FixedString<'a>> for $AString {
             fn eq(&self, other: &$FixedString<'a>) -> bool {
                 self.eq(&**other)
             }
         }
 
+        #[repr(C)]
         pub struct $String<'a> {
-            hdr: $StringRepr,
+            data: *const $char_t,
+            length: u32,
+            flags: u32,
             _marker: PhantomData<&'a [$char_t]>,
         }
 
         impl $String<'static> {
             pub fn new() -> $String<'static> {
                 $String {
-                    hdr: $StringRepr {
-                        data: ptr::null(),
-                        length: 0,
-                        flags: F_NONE,
-                    },
+                    data: ptr::null(),
+                    length: 0,
+                    flags: F_NONE,
                     _marker: PhantomData,
                 }
             }
         }
 
         impl<'a> Deref for $String<'a> {
             type Target = $AString;
             fn deref(&self) -> &$AString {
-                &self.hdr
+                unsafe {
+                    mem::transmute(self)
+                }
             }
         }
 
         impl<'a> DerefMut for $String<'a> {
             fn deref_mut(&mut self) -> &mut $AString {
-                &mut self.hdr
+                unsafe {
+                    mem::transmute(self)
+                }
             }
         }
 
         impl<'a> AsRef<[$char_t]> for $String<'a> {
             fn as_ref(&self) -> &[$char_t] {
                 &self
             }
         }
@@ -336,21 +263,19 @@ macro_rules! define_string_types {
                 $String::from(&s[..])
             }
         }
 
         impl<'a> From<&'a [$char_t]> for $String<'a> {
             fn from(s: &'a [$char_t]) -> $String<'a> {
                 assert!(s.len() < (u32::MAX as usize));
                 $String {
-                    hdr: $StringRepr {
-                        data: s.as_ptr(),
-                        length: s.len() as u32,
-                        flags: F_NONE,
-                    },
+                    data: s.as_ptr(),
+                    length: s.len() as u32,
+                    flags: F_NONE,
                     _marker: PhantomData,
                 }
             }
         }
 
         impl From<Box<[$char_t]>> for $String<'static> {
             fn from(s: Box<[$char_t]>) -> $String<'static> {
                 assert!(s.len() < (u32::MAX as usize));
@@ -358,21 +283,19 @@ macro_rules! define_string_types {
                 // a Box<[$char_t]>. this is only safe because in the Gecko
                 // tree, we use the same allocator for Rust code as for C++
                 // code, meaning that our box can be legally freed with
                 // libc::free().
                 let length = s.len() as u32;
                 let ptr = s.as_ptr();
                 mem::forget(s);
                 $String {
-                    hdr: $StringRepr {
-                        data: ptr,
-                        length: length,
-                        flags: F_OWNED,
-                    },
+                    data: ptr,
+                    length: length,
+                    flags: F_OWNED,
                     _marker: PhantomData,
                 }
             }
         }
 
         impl From<Vec<$char_t>> for $String<'static> {
             fn from(s: Vec<$char_t>) -> $String<'static> {
                 s.into_boxed_slice().into()
@@ -430,62 +353,56 @@ macro_rules! define_string_types {
         }
 
         impl<'a, 'b> cmp::PartialEq<&'b str> for $String<'a> {
             fn eq(&self, other: &&'b str) -> bool {
                 $AString::eq(self, *other)
             }
         }
 
-        impl<'a> Drop for $String<'a> {
-            fn drop(&mut self) {
-                unsafe {
-                    self.finalize();
-                }
-            }
-        }
-
         /// A nsFixed[C]String is a string which uses a fixed size mutable
         /// backing buffer for storing strings which will fit within that
         /// buffer, rather than using heap allocations.
+        #[repr(C)]
         pub struct $FixedString<'a> {
-            hdr: $FixedStringRepr,
+            base: $String<'a>,
+            capacity: u32,
+            buffer: *mut $char_t,
             _marker: PhantomData<&'a mut [$char_t]>,
         }
 
         impl<'a> $FixedString<'a> {
             pub fn new(buf: &'a mut [$char_t]) -> $FixedString<'a> {
                 let len = buf.len();
                 assert!(len < (u32::MAX as usize));
                 let buf_ptr = buf.as_mut_ptr();
                 $FixedString {
-                    hdr: $FixedStringRepr {
-                        base: $StringRepr {
-                            data: ptr::null(),
-                            length: 0,
-                            flags: F_CLASS_FIXED,
-                        },
-                        capacity: len as u32,
-                        buffer: buf_ptr,
+                    base: $String {
+                        data: ptr::null(),
+                        length: 0,
+                        flags: F_CLASS_FIXED,
+                        _marker: PhantomData,
                     },
+                    capacity: len as u32,
+                    buffer: buf_ptr,
                     _marker: PhantomData,
                 }
             }
         }
 
         impl<'a> Deref for $FixedString<'a> {
             type Target = $AString;
             fn deref(&self) -> &$AString {
-                &self.hdr.base
+                &self.base
             }
         }
 
         impl<'a> DerefMut for $FixedString<'a> {
             fn deref_mut(&mut self) -> &mut $AString {
-                &mut self.hdr.base
+                &mut self.base
             }
         }
 
         impl<'a> AsRef<[$char_t]> for $FixedString<'a> {
             fn as_ref(&self) -> &[$char_t] {
                 &self
             }
         }
@@ -532,51 +449,32 @@ macro_rules! define_string_types {
             }
         }
 
         impl<'a, 'b> cmp::PartialEq<&'b str> for $FixedString<'a> {
             fn eq(&self, other: &&'b str) -> bool {
                 $AString::eq(self, *other)
             }
         }
-
-        impl<'a> Drop for $FixedString<'a> {
-            fn drop(&mut self) {
-                unsafe {
-                    self.finalize();
-                }
-            }
-        }
     }
 }
 
 ///////////////////////////////////////////
 // Bindings for nsCString (u8 char type) //
 ///////////////////////////////////////////
 
 define_string_types! {
     char_t = u8;
 
     AString = nsACString;
     String = nsCString;
     FixedString = nsFixedCString;
-
-    StringRepr = nsCStringRepr;
-    FixedStringRepr = nsFixedCStringRepr;
-    AutoStringRepr = nsAutoCStringRepr;
 }
 
 impl nsACString {
-    /// Leaves the nsACString in an unstable state with a dangling data pointer.
-    /// Should only be used in drop implementations of rust types which wrap
-    /// this type.
-    unsafe fn finalize(&mut self) {
-        Gecko_FinalizeCString(self);
-    }
-
     pub fn assign<T: AsRef<[u8]> + ?Sized>(&mut self, other: &T) {
         let s = nsCString::from(other.as_ref());
         unsafe {
             Gecko_AssignCString(self, &*s);
         }
     }
 
     pub fn assign_utf16<T: AsRef<[u16]> + ?Sized>(&mut self, other: &T) {
@@ -598,16 +496,24 @@ impl nsACString {
         }
     }
 
     pub unsafe fn as_str_unchecked(&self) -> &str {
         str::from_utf8_unchecked(self)
     }
 }
 
+impl<'a> Drop for nsCString<'a> {
+    fn drop(&mut self) {
+        unsafe {
+            Gecko_FinalizeCString(&mut **self);
+        }
+    }
+}
+
 impl<'a> From<&'a str> for nsCString<'a> {
     fn from(s: &'a str) -> nsCString<'a> {
         s.as_bytes().into()
     }
 }
 
 impl From<Box<str>> for nsCString<'static> {
     fn from(s: Box<str>) -> nsCString<'static> {
@@ -660,30 +566,19 @@ macro_rules! ns_auto_cstring {
 ///////////////////////////////////////////
 
 define_string_types! {
     char_t = u16;
 
     AString = nsAString;
     String = nsString;
     FixedString = nsFixedString;
-
-    StringRepr = nsStringRepr;
-    FixedStringRepr = nsFixedStringRepr;
-    AutoStringRepr = nsAutoStringRepr;
 }
 
 impl nsAString {
-    /// Leaves the nsAString in an unstable state with a dangling data pointer.
-    /// Should only be used in drop implementations of rust types which wrap
-    /// this type.
-    unsafe fn finalize(&mut self) {
-        Gecko_FinalizeString(self);
-    }
-
     pub fn assign<T: AsRef<[u16]> + ?Sized>(&mut self, other: &T) {
         let s = nsString::from(other.as_ref());
         unsafe {
             Gecko_AssignString(self, &*s);
         }
     }
 
     pub fn assign_utf8<T: AsRef<[u8]> + ?Sized>(&mut self, other: &T) {
@@ -701,16 +596,24 @@ impl nsAString {
     pub fn append_utf8<T: AsRef<[u8]> + ?Sized>(&mut self, other: &T) {
         let s = nsCString::from(other.as_ref());
         unsafe {
             Gecko_AppendUTF8toString(self, &*s);
         }
     }
 }
 
+impl<'a> Drop for nsString<'a> {
+    fn drop(&mut self) {
+        unsafe {
+            Gecko_FinalizeString(&mut **self);
+        }
+    }
+}
+
 // NOTE: The From impl for a string slice for nsString produces a <'static>
 // lifetime, as it allocates.
 impl<'a> From<&'a str> for nsString<'static> {
     fn from(s: &'a str) -> nsString<'static> {
         s.encode_utf16().collect::<Vec<u16>>().into()
     }
 }
 
@@ -774,20 +677,20 @@ extern "C" {
 pub mod test_helpers {
     //! This module only exists to help with ensuring that the layout of the
     //! structs inside of rust and C++ are identical.
     //!
     //! It is public to ensure that these testing functions are avaliable to
     //! gtest code.
 
     use super::{
-        nsFixedCStringRepr,
-        nsFixedStringRepr,
-        nsCStringRepr,
-        nsStringRepr,
+        nsFixedCString,
+        nsFixedString,
+        nsCString,
+        nsString,
         F_NONE,
         F_TERMINATED,
         F_VOIDED,
         F_SHARED,
         F_OWNED,
         F_FIXED,
         F_LITERAL,
         F_CLASS_FIXED,
@@ -804,20 +707,20 @@ pub mod test_helpers {
                 unsafe {
                     *size = mem::size_of::<$T>();
                     *align = mem::align_of::<$T>();
                 }
             }
         }
     }
 
-    size_align_check!(nsStringRepr, Rust_Test_ReprSizeAlign_nsString);
-    size_align_check!(nsCStringRepr, Rust_Test_ReprSizeAlign_nsCString);
-    size_align_check!(nsFixedStringRepr, Rust_Test_ReprSizeAlign_nsFixedString);
-    size_align_check!(nsFixedCStringRepr, Rust_Test_ReprSizeAlign_nsFixedCString);
+    size_align_check!(nsString<'static>, Rust_Test_ReprSizeAlign_nsString);
+    size_align_check!(nsCString<'static>, Rust_Test_ReprSizeAlign_nsCString);
+    size_align_check!(nsFixedString<'static>, Rust_Test_ReprSizeAlign_nsFixedString);
+    size_align_check!(nsFixedCString<'static>, Rust_Test_ReprSizeAlign_nsFixedCString);
 
     /// Generates a $[no_mangle] extern "C" function which returns the size,
     /// alignment and offset in the parent struct of a given member, with the
     /// given name.
     ///
     /// This method can trigger Undefined Behavior if the accessing the member
     /// $member on a given type would use that type's `Deref` implementation.
     macro_rules! member_check {
@@ -837,26 +740,26 @@ pub mod test_helpers {
                         (&tmp.$member as *const _ as usize) -
                         (&tmp as *const _ as usize);
                     mem::forget(tmp);
                 }
             }
         }
     }
 
-    member_check!(nsStringRepr, data, Rust_Test_Member_nsString_mData);
-    member_check!(nsStringRepr, length, Rust_Test_Member_nsString_mLength);
-    member_check!(nsStringRepr, flags, Rust_Test_Member_nsString_mFlags);
-    member_check!(nsCStringRepr, data, Rust_Test_Member_nsCString_mData);
-    member_check!(nsCStringRepr, length, Rust_Test_Member_nsCString_mLength);
-    member_check!(nsCStringRepr, flags, Rust_Test_Member_nsCString_mFlags);
-    member_check!(nsFixedStringRepr, capacity, Rust_Test_Member_nsFixedString_mFixedCapacity);
-    member_check!(nsFixedStringRepr, buffer, Rust_Test_Member_nsFixedString_mFixedBuf);
-    member_check!(nsFixedCStringRepr, capacity, Rust_Test_Member_nsFixedCString_mFixedCapacity);
-    member_check!(nsFixedCStringRepr, buffer, Rust_Test_Member_nsFixedCString_mFixedBuf);
+    member_check!(nsString<'static>, data, Rust_Test_Member_nsString_mData);
+    member_check!(nsString<'static>, length, Rust_Test_Member_nsString_mLength);
+    member_check!(nsString<'static>, flags, Rust_Test_Member_nsString_mFlags);
+    member_check!(nsCString<'static>, data, Rust_Test_Member_nsCString_mData);
+    member_check!(nsCString<'static>, length, Rust_Test_Member_nsCString_mLength);
+    member_check!(nsCString<'static>, flags, Rust_Test_Member_nsCString_mFlags);
+    member_check!(nsFixedString<'static>, capacity, Rust_Test_Member_nsFixedString_mFixedCapacity);
+    member_check!(nsFixedString<'static>, buffer, Rust_Test_Member_nsFixedString_mFixedBuf);
+    member_check!(nsFixedCString<'static>, capacity, Rust_Test_Member_nsFixedCString_mFixedCapacity);
+    member_check!(nsFixedCString<'static>, buffer, Rust_Test_Member_nsFixedCString_mFixedBuf);
 
     #[no_mangle]
     #[allow(non_snake_case)]
     pub extern fn Rust_Test_NsStringFlags(f_none: *mut u32,
                                           f_terminated: *mut u32,
                                           f_voided: *mut u32,
                                           f_shared: *mut u32,
                                           f_owned: *mut u32,