Slightly improve InternedString

* Use `&'static str` instead of (ptr, len) pair to reduce unsafety.
* try make hash calculation O(1) instead of O(n), fail miserably,
  document findings.
* Rename `to_inner` -> `as_str()`.

diff --git a/src/cargo/core/interning.rs b/src/cargo/core/interning.rs
index a55e684c95aae187a066d7f203041d10cc209083..2f8cf7451fa4eb6d2bd0a9152cd83f5819b20504 100644 (file)
--- a/src/cargo/core/interning.rs
+++ b/src/cargo/core/interning.rs
@@ -4,6 +4,7 @@ use std::collections::HashSet;
 use std::slice;
 use std::str;
 use std::mem;
+use std::ptr;
 use std::cmp::Ordering;
 use std::ops::Deref;
 use std::hash::{Hash, Hasher};
@@ -24,33 +25,33 @@ lazy_static! {
         RwLock::new(HashSet::new());
 }
 
-#[derive(Eq, PartialEq, Clone, Copy)]
+#[derive(Clone, Copy)]
 pub struct InternedString {
-    ptr: *const u8,
-    len: usize,
+    inner: &'static str,
 }
 
+impl PartialEq for InternedString {
+    fn eq(&self, other: &InternedString) -> bool {
+        ptr::eq(self.as_str(), other.as_str())
+    }
+}
+
+impl Eq for InternedString {}
+
 impl InternedString {
     pub fn new(str: &str) -> InternedString {
         let mut cache = STRING_CACHE.write().unwrap();
-        if let Some(&s) = cache.get(str) {
-            return InternedString {
-                ptr: s.as_ptr(),
-                len: s.len(),
-            };
-        }
-        let s = leak(str.to_string());
-        cache.insert(s);
-        InternedString {
-            ptr: s.as_ptr(),
-            len: s.len(),
-        }
+        let s = cache.get(str).map(|&s| s).unwrap_or_else(|| {
+            let s = leak(str.to_string());
+            cache.insert(s);
+            s
+        });
+
+        InternedString { inner: s }
     }
-    pub fn to_inner(&self) -> &'static str {
-        unsafe {
-            let slice = slice::from_raw_parts(self.ptr, self.len);
-            &str::from_utf8_unchecked(slice)
-        }
+
+    pub fn as_str(&self) -> &'static str {
+        self.inner
     }
 }
 
@@ -58,31 +59,34 @@ impl Deref for InternedString {
     type Target = str;
 
     fn deref(&self) -> &'static str {
-        self.to_inner()
+        self.as_str()
     }
 }
 
 impl Hash for InternedString {
+    // NB: we can't implement this as `identity(self).hash(state)`,
+    // because we use this for on-disk fingerprints and so need
+    // stability across Cargo invocations.
     fn hash<H: Hasher>(&self, state: &mut H) {
-        self.to_inner().hash(state);
+        self.as_str().hash(state);
     }
 }
 
 impl fmt::Debug for InternedString {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        fmt::Debug::fmt(self.to_inner(), f)
+        fmt::Debug::fmt(self.as_str(), f)
     }
 }
 
 impl fmt::Display for InternedString {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        fmt::Display::fmt(self.to_inner(), f)
+        fmt::Display::fmt(self.as_str(), f)
     }
 }
 
 impl Ord for InternedString {
     fn cmp(&self, other: &InternedString) -> Ordering {
-        self.to_inner().cmp(&*other)
+        self.as_str().cmp(other.as_str())
     }
 }
 
@@ -91,6 +95,3 @@ impl PartialOrd for InternedString {
         Some(self.cmp(other))
     }
 }
-
-unsafe impl Send for InternedString {}
-unsafe impl Sync for InternedString {}

diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs
index eaf7036d97f2ce0d813bd8d8771bfc147e22b57c..a1f97fe3c251b48d5048776d97d4336b93d91e08 100644 (file)
--- a/src/cargo/core/resolver/context.rs
+++ b/src/cargo/core/resolver/context.rs
@@ -280,7 +280,7 @@ fn build_requirements<'a, 'b: 'a>(
                 reqs.require_feature(key)?;
             }
             for dep in s.dependencies().iter().filter(|d| d.is_optional()) {
-                reqs.require_dependency(dep.name().to_inner());
+                reqs.require_dependency(dep.name().as_str());
             }
         }
         Method::Required {

diff --git a/src/cargo/ops/cargo_doc.rs b/src/cargo/ops/cargo_doc.rs
index 45c51974bb2471e481a37eeb2a12afb3047c8cbc..647b639ab8719ec27e57d6c03215bb1e88fc7da1 100644 (file)
--- a/src/cargo/ops/cargo_doc.rs
+++ b/src/cargo/ops/cargo_doc.rs
@@ -69,7 +69,7 @@ pub fn doc(ws: &Workspace, options: &DocOptions) -> CargoResult<()> {
                  Please re-run this command with `-p <spec>` where `<spec>` \
                  is one of the following:\n  {}",
                 pkgs.iter()
-                    .map(|p| p.name().to_inner())
+                    .map(|p| p.name().as_str())
                     .collect::<Vec<_>>()
                     .join("\n  ")
             );

diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs
index 741974e3b3521bcee82a21c0c1d234b3e28f6aa5..ae0cc7cbb53337c0a54be3ab5e7db09fde136b2b 100644 (file)
--- a/src/cargo/ops/cargo_generate_lockfile.rs
+++ b/src/cargo/ops/cargo_generate_lockfile.rs
@@ -142,7 +142,7 @@ pub fn update_lockfile(ws: &Workspace, opts: &UpdateOptions) -> CargoResult<()>
         resolve: &'a Resolve,
     ) -> Vec<(Vec<&'a PackageId>, Vec<&'a PackageId>)> {
         fn key(dep: &PackageId) -> (&str, &SourceId) {
-            (dep.name().to_inner(), dep.source_id())
+            (dep.name().as_str(), dep.source_id())
         }
 
         // Removes all package ids in `b` from `a`. Note that this is somewhat

diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs
index 10449b95e4f1d25e4b379d6762aaa6e3f1a44c19..232c804e8f5c2ef611c781156f796eacebd2965b 100644 (file)
--- a/src/cargo/ops/cargo_install.rs
+++ b/src/cargo/ops/cargo_install.rs
@@ -506,7 +506,7 @@ where
                     "multiple packages with {} found: {}",
                     kind,
                     pkgs.iter()
-                        .map(|p| p.name().to_inner())
+                        .map(|p| p.name().as_str())
                         .collect::<Vec<_>>()
                         .join(", ")
                 )