[inspector] Fix and refactor String16

src/inspector/string-16.h

Index: src/inspector/string-16.h
diff --git a/src/inspector/string-16.h b/src/inspector/string-16.h
index 6dc7759de059e1151731da35177c05a47826540f..d9cf40baf948d9fb84cf0bddfa72b7a53a6a494d 100644
--- a/src/inspector/string-16.h
+++ b/src/inspector/string-16.h
@@ -21,7 +21,10 @@ class String16 {
   static const size_t kNotFound = static_cast<size_t>(-1);
 
   String16() {}
-  String16(const String16& other) : m_impl(other.m_impl) {}
+  String16(const String16& other)
+      : m_impl(other.m_impl), hash_code(other.hash_code) {}
+  String16(const String16&& other)
+      : m_impl(std::move(other.m_impl)), hash_code(other.hash_code) {}
   String16(const UChar* characters, size_t size) : m_impl(characters, size) {}
   String16(const UChar* characters)  // NOLINT(runtime/explicit)
       : m_impl(characters) {}
@@ -31,6 +34,9 @@ class String16 {
     m_impl.resize(size);
     for (size_t i = 0; i < size; ++i) m_impl[i] = characters[i];
   }
+  explicit String16(const std::basic_string<UChar>& impl) : m_impl(impl) {}
+
+  String16& operator=(const String16& other) = default;

[dgozman, 2016/11/11 21:41:21]

Let's have operator=(const String16&&) as well?

[Clemens Hammacher, 2016/11/14 06:02:07]

Sure, I will add it when rebasing.

 
   static String16 fromInteger(int);
   static String16 fromInteger(size_t);
@@ -52,51 +58,53 @@ class String16 {
   size_t reverseFind(const String16& str, size_t start = UINT_MAX) const {
     return m_impl.rfind(str.m_impl, start);
   }
-  void swap(String16& other) { m_impl.swap(other.m_impl); }
+  size_t find(UChar c, size_t start = 0) const { return m_impl.find(c, start); }
+  size_t reverseFind(UChar c, size_t start = UINT_MAX) const {
+    return m_impl.rfind(c, start);
+  }
+  void swap(String16& other) {
+    m_impl.swap(other.m_impl);
+    std::swap(hash_code, other.hash_code);
+  }
 
   // Convenience methods.
   std::string utf8() const;
   static String16 fromUTF8(const char* stringStart, size_t length);
 
-  const std::basic_string<UChar>& impl() const { return m_impl; }
-  explicit String16(const std::basic_string<UChar>& impl) : m_impl(impl) {}
-
   std::size_t hash() const {
-    if (!has_hash) {
-      size_t hash = 0;
-      for (size_t i = 0; i < length(); ++i) hash = 31 * hash + m_impl[i];
-      hash_code = hash;
-      has_hash = true;
+    if (!hash_code) {
+      for (char c : m_impl) hash_code = 31 * hash_code + c;
+      // Map hash code 0 to 1. This double the number of hash collisions for 1,
+      // but avoids recomputing the hash code.
+      if (!hash_code) ++hash_code;

[dgozman, 2016/11/11 21:41:21]

What if hash_code == -1?

[Clemens Hammacher, 2016/11/14 06:02:07]

Then !hash_code will be false, so it stays -1.

[dgozman, 2016/11/14 20:32:52]

Ops, my bad :-)

     }
     return hash_code;
   }
 
+  inline bool operator==(const String16& other) const {
+    return m_impl == other.m_impl;
+  }
+  inline bool operator<(const String16& other) const {
+    return m_impl < other.m_impl;
+  }
+  inline bool operator!=(const String16& other) const {
+    return m_impl != other.m_impl;
+  }
+  inline String16 operator+(const String16& other) const {
+    return String16(m_impl + other.m_impl);
+  }
+
+  // Defined later, since it uses the String16Builder.
+  template <typename... T>
+  static String16 concat(T... args);
+
  private:
   std::basic_string<UChar> m_impl;
-  mutable bool has_hash = false;
   mutable std::size_t hash_code = 0;
 };
 
-inline bool operator==(const String16& a, const String16& b) {
-  return a.impl() == b.impl();
-}
-inline bool operator<(const String16& a, const String16& b) {
-  return a.impl() < b.impl();
-}
-inline bool operator!=(const String16& a, const String16& b) {
-  return a.impl() != b.impl();
-}
-inline bool operator==(const String16& a, const char* b) {
-  return a.impl() == String16(b).impl();
-}
-inline String16 operator+(const String16& a, const char* b) {

[dgozman, 2016/11/11 21:41:21]

Where did this one go?

[Clemens Hammacher, 2016/11/14 06:02:07]

It's redundant, since there is a String16(const char*) constructor. So String16
+ const char* is the same String16 + String16(const char*) anyway.

-  return String16(a.impl() + String16(b).impl());
-}
 inline String16 operator+(const char* a, const String16& b) {
-  return String16(String16(a).impl() + b.impl());
-}
-inline String16 operator+(const String16& a, const String16& b) {
-  return String16(a.impl() + b.impl());
+  return String16(a) + b;
 }
 
 class String16Builder {
@@ -107,13 +115,29 @@ class String16Builder {
   void append(char);
   void append(const UChar*, size_t);
   void append(const char*, size_t);
+  void append(int);

[dgozman, 2016/11/11 21:41:21]

I think these are error-prone, since the semantic is really different from
append(UChar), but the signature is similar. Let's call it appendNumber(...).

[Clemens Hammacher, 2016/11/14 06:02:08]

I see the problem. Renaming the functions would make the appendAll magic more
difficult though, requiring template specialization for all types where the
append function has a different name. Is this worth it? For the appendAll
function, the user has to be careful anyway not to confuse int or size_t with
UChar.

[dgozman, 2016/11/14 20:32:52]

Maybe don't support integer than? Why not pass String::fromInteger(x) instead,
even to appendAll?

[Clemens Hammacher, 2016/11/14 21:11:02]

Hm, that degrades performance and readability, which also propagates to the
String16::concat method. See v8-debugger-agent-impl.cc for an example that would
be much more verbose with explicit String16::fromInteger calls.
Do you insist on this change? ;) 

+  void append(size_t);
   String16 toString();
   void reserveCapacity(size_t);
 
+  template <typename T, typename... R>
+  void appendAll(T first, R... rest) {
+    append(first);
+    appendAll(rest...);
+  }
+  void appendAll() {}
+
  private:
   std::vector<UChar> m_buffer;
 };
 
+template <typename... T>
+String16 String16::concat(T... args) {
+  String16Builder builder;
+  builder.appendAll(args...);
+  return builder.toString();
+}
+
 }  // namespace v8_inspector
 
 #if !defined(__APPLE__) || defined(_LIBCPP_VERSION)