De-duplicate common code from StringPiece, StringPiece16, and their tests.

base/string_piece.h

Index: base/string_piece.h
diff --git a/base/string_piece.h b/base/string_piece.h
index 278c7b692ae4750673dee32abcc2cc69c1631d3b..4958a7b477c27d13376a9bbca1149ca7decc3309 100644
--- a/base/string_piece.h
+++ b/base/string_piece.h
@@ -33,17 +33,19 @@
 
 namespace base {
 
-class BASE_EXPORT StringPiece {
+namespace detail {

[brettw, 2011/12/01 20:29:24]

Normally this would be "internal" or something, but I don't actually think it's
necessary in this case. I think it's fine for this to just be in the base
namespace (just like basic_string is in std).

[erikwright (departed), 2011/12/06 15:52:06]

A compelling reason for basic_string to be in std is because you might template
a client class, method, or function on basic_string<CHAR>. The original CL here
did not really support that, since StringPiece was a subclass of
BasicStringPiece while StringPiece16 was a direct instantiation.

I modified things so that both StringPiece and StringPiece16 are typedef'd to
BasicStringPiece<std::string/string16>, which is itself a subclass of
StringPieceDetail. The common behaviour is in StringPieceDetail while
BasicStringPiece is specialized to add functionality to the single-byte
versions.

With that in mind, I would propose that clients should never refer directly to
StringPieceDetail, and it therefore belongs in an 'internal' namespace.

+
+template <typename STRING_TYPE> class BasicStringPiece {
  public:
   // standard STL container boilerplate
   typedef size_t size_type;
-  typedef char value_type;
-  typedef const char* pointer;
-  typedef const char& reference;
-  typedef const char& const_reference;
+  typedef typename STRING_TYPE::value_type value_type;
+  typedef const value_type* pointer;
+  typedef const value_type& reference;
+  typedef const value_type& const_reference;
   typedef ptrdiff_t difference_type;
-  typedef const char* const_iterator;
-  typedef const char* iterator;
+  typedef const value_type* const_iterator;
+  typedef const value_type* iterator;
   typedef std::reverse_iterator<const_iterator> const_reverse_iterator;
   typedef std::reverse_iterator<iterator> reverse_iterator;

[brettw, 2011/12/01 20:29:24]

I'd make this also const to match the non-reverse iterator (everything is a
const iterator in StringPiece).

[erikwright (departed), 2011/12/06 15:52:06]

In fact, why not just remove the non-const typedef's, since they are not used
(if I follow the logical conclusions of your following comment).

 
@@ -52,20 +54,21 @@ class BASE_EXPORT StringPiece {
  public:
   // We provide non-explicit singleton constructors so users can pass
   // in a "const char*" or a "string" wherever a "StringPiece" is
-  // expected.
-  StringPiece() : ptr_(NULL), length_(0) { }
-  StringPiece(const char* str)
-    : ptr_(str), length_((str == NULL) ? 0 : strlen(str)) { }
-  StringPiece(const std::string& str)
+  // expected (likewise for char16, string16, StringPiece16).
+  BasicStringPiece() : ptr_(NULL), length_(0) { }
+  BasicStringPiece(const value_type* str)
+    : ptr_(str),
+      length_((str == NULL) ? 0 : STRING_TYPE::traits_type::length(str)) { }
+  BasicStringPiece(const STRING_TYPE& str)
     : ptr_(str.data()), length_(str.size()) { }
-  StringPiece(const char* offset, size_type len)
+  BasicStringPiece(const value_type* offset, size_type len)
     : ptr_(offset), length_(len) { }
 
   // data() may return a pointer to a buffer with embedded NULs, and the
   // returned buffer may or may not be null terminated.  Therefore it is
   // typically a mistake to pass data() to a routine that expects a NUL
   // terminated string.
-  const char* data() const { return ptr_; }
+  const value_type* data() const { return ptr_; }
   size_type size() const { return length_; }
   size_type length() const { return length_; }
   bool empty() const { return length_ == 0; }
@@ -74,20 +77,16 @@ class BASE_EXPORT StringPiece {
     ptr_ = NULL;
     length_ = 0;
   }
-  void set(const char* data, size_type len) {
+  void set(const value_type* data, size_type len) {
     ptr_ = data;
     length_ = len;
   }
-  void set(const char* str) {
+  void set(const value_type* str) {
     ptr_ = str;
-    length_ = str ? strlen(str) : 0;
-  }
-  void set(const void* data, size_type len) {
-    ptr_ = reinterpret_cast<const char*>(data);
-    length_ = len;
+    length_ = str ? STRING_TYPE::traits_type::length(str) : 0;
   }
 
-  char operator[](size_type i) const { return ptr_[i]; }
+  value_type operator[](size_type i) const { return ptr_[i]; }
 
   void remove_prefix(size_type n) {
     ptr_ += n;
@@ -98,6 +97,50 @@ class BASE_EXPORT StringPiece {
     length_ -= n;
   }
 
+  STRING_TYPE as_string() const {
+    // std::string doesn't like to take a NULL pointer even with a 0 size.
+    return empty() ? STRING_TYPE() : STRING_TYPE(data(), size());
+  }
+
+  iterator begin() const { return ptr_; }

[brettw, 2011/12/01 20:29:24]

Can these return const_iterator instead? The return type will be the same, but I
think it's best if everything matches.

[erikwright (departed), 2011/12/06 15:52:06]

Sure. Given this, I guess the next step would be to simply remove iterator and
reverse_iterator from the definition, as they are no longer used?

+  iterator end() const { return ptr_ + length_; }
+  const_reverse_iterator rbegin() const {
+    return const_reverse_iterator(ptr_ + length_);
+  }
+  const_reverse_iterator rend() const {
+    return const_reverse_iterator(ptr_);
+  }
+
+  size_type max_size() const { return length_; }
+  size_type capacity() const { return length_; }
+
+  static int wordmemcmp(const value_type* p,
+                        const value_type* p2,
+                        size_type N) {
+    return STRING_TYPE::traits_type::compare(p, p2, N);
+  }
+
+ protected:
+  const value_type* ptr_;
+  size_type     length_;
+};
+
+template <typename STRING_TYPE>
+const typename BasicStringPiece<STRING_TYPE>::size_type
+BasicStringPiece<STRING_TYPE>::npos =
+    typename BasicStringPiece<STRING_TYPE>::size_type(-1);
+
+}  // namespace detail
+
+class BASE_EXPORT StringPiece : public detail::BasicStringPiece<std::string> {
+ public:
+  StringPiece() {}
+  StringPiece(const char* str) : detail::BasicStringPiece<std::string>(str) {}
+  StringPiece(const std::string& str)
+      : detail::BasicStringPiece<std::string>(str) {}
+  StringPiece(const char* offset, size_type len)
+      : detail::BasicStringPiece<std::string>(offset, len) {}
+
   int compare(const StringPiece& x) const {
     int r = wordmemcmp(
         ptr_, x.ptr_, (length_ < x.length_ ? length_ : x.length_));
@@ -108,9 +151,9 @@ class BASE_EXPORT StringPiece {
     return r;
   }
 
-  std::string as_string() const {
-    // std::string doesn't like to take a NULL pointer even with a 0 size.
-    return std::string(!empty() ? data() : "", size());
+  void set(const void* data, size_type len) {
+    ptr_ = reinterpret_cast<const value_type*>(data);
+    length_ = len;
   }
 
   void CopyToString(std::string* target) const;
@@ -128,18 +171,6 @@ class BASE_EXPORT StringPiece {
             (wordmemcmp(ptr_ + (length_-x.length_), x.ptr_, x.length_) == 0));
   }
 
-  iterator begin() const { return ptr_; }
-  iterator end() const { return ptr_ + length_; }
-  const_reverse_iterator rbegin() const {
-    return const_reverse_iterator(ptr_ + length_);
-  }
-  const_reverse_iterator rend() const {
-    return const_reverse_iterator(ptr_);
-  }
-
-  size_type max_size() const { return length_; }
-  size_type capacity() const { return length_; }
-
   size_type copy(char* buf, size_type n, size_type pos = 0) const;
 
   size_type find(const StringPiece& s, size_type pos = 0) const;
@@ -161,93 +192,9 @@ class BASE_EXPORT StringPiece {
   size_type find_last_not_of(char c, size_type pos = npos) const;
 
   StringPiece substr(size_type pos, size_type n = npos) const;
-
-  static int wordmemcmp(const char* p, const char* p2, size_type N) {
-    return memcmp(p, p2, N);
-  }
-
- private:
-  const char*   ptr_;
-  size_type     length_;
 };
 
-class BASE_EXPORT StringPiece16 {
- public:
-  // standard STL container boilerplate
-  typedef size_t size_type;
-  typedef char16 value_type;
-  typedef const char16* pointer;
-  typedef const char16& reference;
-  typedef const char16& const_reference;
-  typedef ptrdiff_t difference_type;
-  typedef const char16* const_iterator;
-  typedef const char16* iterator;
-  typedef std::reverse_iterator<const_iterator> const_reverse_iterator;
-  typedef std::reverse_iterator<iterator> reverse_iterator;
-
- public:
-  // We provide non-explicit singleton constructors so users can pass
-  // in a "const char16*" or a "string16" wherever a "StringPiece16" is
-  // expected.
-  StringPiece16() : ptr_(NULL), length_(0) { }
-  StringPiece16(const char16* str)
-      : ptr_(str),
-        length_((str == NULL) ? 0 : string16::traits_type::length(str)) { }
-  StringPiece16(const string16& str)
-      : ptr_(str.data()), length_(str.size()) { }
-  StringPiece16(const char16* offset, size_type len)
-      : ptr_(offset), length_(len) { }
-
-  // data() may return a pointer to a buffer with embedded NULs, and the
-  // returned buffer may or may not be null terminated.  Therefore it is
-  // typically a mistake to pass data() to a routine that expects a NUL
-  // terminated string.
-  const char16* data() const { return ptr_; }
-  size_type size() const { return length_; }
-  size_type length() const { return length_; }
-  bool empty() const { return length_ == 0; }
-
-  void clear() {
-    ptr_ = NULL;
-    length_ = 0;
-  }
-  void set(const char16* data, size_type len) {
-    ptr_ = data;
-    length_ = len;
-  }
-  void set(const char16* str) {
-    ptr_ = str;
-    length_ = str ? string16::traits_type::length(str) : 0;
-  }
-
-  char16 operator[](size_type i) const { return ptr_[i]; }
-
-  string16 as_string16() const {
-    // StringPiece claims that this is bad when data() is NULL, but unittesting
-    // seems to say otherwise.
-    return string16(data(), size());
-  }
-
-  iterator begin() const { return ptr_; }
-  iterator end() const { return ptr_ + length_; }
-  const_reverse_iterator rbegin() const {
-    return const_reverse_iterator(ptr_ + length_);
-  }
-  const_reverse_iterator rend() const {
-    return const_reverse_iterator(ptr_);
-  }
-
-  size_type max_size() const { return length_; }
-  size_type capacity() const { return length_; }
-
-  static int wordmemcmp(const char16* p, const char16* p2, size_type N) {
-    return string16::traits_type::compare(p, p2, N);
-  }
-
- private:
-  const char16*   ptr_;
-  size_type       length_;
-};
+typedef detail::BasicStringPiece<string16> StringPiece16;
 
 BASE_EXPORT bool operator==(const StringPiece& x, const StringPiece& y);