ake string_util::WriteInto() DCHECK() that the supplied |length_with_null| > 1, meaning that the without-'\0' string is non-empty. This replaces the conditional code added recently that makes this case return NULL. It's easier to understand if it's simply an error to call WriteInto() in this case at all.

Add DCHECK()s or conditionals as appropriate to callers in order to ensure this assertion holds.

BUG=none
TEST=none
Review URL: http://codereview.chromium.org/8418034

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@112005 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 51 insertions, 70 deletions

diff --git a/base/base_paths_mac.mm b/base/base_paths_mac.mm
index a9cd471b..aed3ea8 100644
--- a/base/base_paths_mac.mm
+++ b/base/base_paths_mac.mm
@@ -18,24 +18,18 @@
 
 namespace {
 
-bool GetNSExecutablePath(FilePath* path) WARN_UNUSED_RESULT;
-
-bool GetNSExecutablePath(FilePath* path) {
+void GetNSExecutablePath(FilePath* path) {
   DCHECK(path);
   // Executable path can have relative references ("..") depending on
   // how the app was launched.
   uint32_t executable_length = 0;
   _NSGetExecutablePath(NULL, &executable_length);
-  DCHECK_GE(executable_length, 1u);
+  DCHECK_GT(executable_length, 1u);
   std::string executable_path;
-  char* executable_path_c = WriteInto(&executable_path, executable_length);
-  int rv = _NSGetExecutablePath(executable_path_c, &executable_length);
+  int rv = _NSGetExecutablePath(WriteInto(&executable_path, executable_length),
+                                &executable_length);
   DCHECK_EQ(rv, 0);
-  DCHECK(!executable_path.empty());
-  if ((rv != 0) || (executable_path.empty()))
-    return false;
   *path = FilePath(executable_path);
-  return true;
 }
 
 // Returns true if the module for |address| is found. |path| will contain
@@ -58,7 +52,8 @@ namespace base {
 bool PathProviderMac(int key, FilePath* result) {
   switch (key) {
     case base::FILE_EXE:
-      return GetNSExecutablePath(result);
+      GetNSExecutablePath(result);
+      return true;
     case base::FILE_MODULE:
       return GetModulePathForAddress(result,
           reinterpret_cast<const void*>(&base::PathProviderMac));
diff --git a/base/rand_util.cc b/base/rand_util.cc
index fcbccef..a9bc961 100644
--- a/base/rand_util.cc
+++ b/base/rand_util.cc
@@ -71,6 +71,7 @@ void RandBytes(void* output, size_t output_length) {
 }
 
 std::string RandBytesAsString(size_t length) {
+  DCHECK_GT(length, 0u);
   std::string result;
   RandBytes(WriteInto(&result, length + 1), length);
   return result;
diff --git a/base/rand_util.h b/base/rand_util.h
index 4474c02..8d795a3 100644
--- a/base/rand_util.h
+++ b/base/rand_util.h
@@ -38,9 +38,9 @@ BASE_EXPORT double BitsToOpenEndedUnitInterval(uint64 bits);
 BASE_EXPORT void RandBytes(void* output, size_t output_length);
 
 // Fills a string of length |length| with with cryptographically strong random
-// data and returns it.
+// data and returns it.  |length| should be nonzero.
 //
-// Not that this is a variation of |RandBytes| with a different return type.
+// Note that this is a variation of |RandBytes| with a different return type.
 BASE_EXPORT std::string RandBytesAsString(size_t length);
 
 }  // namespace base
diff --git a/base/rand_util_unittest.cc b/base/rand_util_unittest.cc
index a3474ba..e0e85ec 100644
--- a/base/rand_util_unittest.cc
+++ b/base/rand_util_unittest.cc
@@ -41,8 +41,8 @@ TEST(RandUtilTest, RandBytes) {
 }
 
 TEST(RandUtilTest, RandBytesAsString) {
-  std::string random_string = base::RandBytesAsString(0);
-  EXPECT_EQ(0U, random_string.size());
+  std::string random_string = base::RandBytesAsString(1);
+  EXPECT_EQ(1U, random_string.size());
   random_string = base::RandBytesAsString(145);
   EXPECT_EQ(145U, random_string.size());
   char accumulator = 0;
diff --git a/base/string_util.h b/base/string_util.h
index c359e73..a135180 100644
--- a/base/string_util.h
+++ b/base/string_util.h
@@ -457,40 +457,32 @@ BASE_EXPORT void ReplaceSubstringsAfterOffset(
     const std::string& find_this,
     const std::string& replace_with);
 
-// Reserves enough memory in |str| to accommodate |length_with_null|
-// characters, sets the size of |str| to |length_with_null - 1| characters,
-// and returns a pointer to the underlying contiguous array of characters.
+// Reserves enough memory in |str| to accommodate |length_with_null| characters,
+// sets the size of |str| to |length_with_null - 1| characters, and returns a
+// pointer to the underlying contiguous array of characters.  This is typically
+// used when calling a function that writes results into a character array, but
+// the caller wants the data to be managed by a string-like object.  It is
+// convenient in that is can be used inline in the call, and fast in that it
+// avoids copying the results of the call from a char* into a string.
 //
-// This is typically used when calling a function that writes results into a
-// character array, but the caller wants the data to be managed by a
-// string-like object.
+// |length_with_null| must be at least 2, since otherwise the underlying string
+// would have size 0, and trying to access &((*str)[0]) in that case can result
+// in a number of problems.
 //
-// |length_with_null| must be >= 1. In the |length_with_null| == 1 case,
-// NULL is returned rather than a pointer to the array, since there is no way
-// to provide access to the underlying character array of a 0-length
-// string-like object without breaking guarantees provided by the C++
-// standards.
-//
-// Internally, this takes linear time because the underlying array needs to
-// be 0-filled for all |length_with_null - 1| * sizeof(character) bytes.
+// Internally, this takes linear time because the resize() call 0-fills the
+// underlying array for potentially all
+// (|length_with_null - 1| * sizeof(string_type::value_type)) bytes.  Ideally we
+// could avoid this aspect of the resize() call, as we expect the caller to
+// immediately write over this memory, but there is no other way to set the size
+// of the string, and not doing that will mean people who access |str| rather
+// than str.c_str() will get back a string of whatever size |str| had on entry
+// to this function (probably 0).
 template <class string_type>
 inline typename string_type::value_type* WriteInto(string_type* str,
                                                    size_t length_with_null) {
-  DCHECK_NE(0u, length_with_null);
+  DCHECK_GT(length_with_null, 1u);
   str->reserve(length_with_null);
   str->resize(length_with_null - 1);
-
-  // If |length_with_null| is 1, calling (*str)[0] is invalid since the
-  // size() is 0. In some implementations this triggers an assertion.
-  //
-  // Trying to access the underlying byte array by casting away const
-  // when calling str->data() or str->c_str() is also incorrect.
-  // Some implementations of basic_string use a copy-on-write approach and
-  // this could end up mutating the data that is shared across multiple string
-  // objects.
-  if (length_with_null <= 1)
-    return NULL;
-
   return &((*str)[0]);
 }
 
diff --git a/base/string_util_unittest.cc b/base/string_util_unittest.cc
index 1ffc59b..a028882 100644
--- a/base/string_util_unittest.cc
+++ b/base/string_util_unittest.cc
@@ -1113,34 +1113,27 @@ TEST(StringUtilTest, ContainsOnlyChars) {
   EXPECT_FALSE(ContainsOnlyChars("123a", "4321"));
 }
 
-TEST(StringUtilTest, WriteInto) {
+class WriteIntoTest : public testing::Test {
+ protected:
+  static void WritesCorrectly(size_t num_chars) {
+    std::string buffer;
+    char kOriginal[] = "supercali";
+    strncpy(WriteInto(&buffer, num_chars + 1), kOriginal, num_chars);
+    // Using std::string(buffer.c_str()) instead of |buffer| truncates the
+    // string at the first \0.
+    EXPECT_EQ(std::string(kOriginal,
+                          std::min(num_chars, arraysize(kOriginal) - 1)),
+              std::string(buffer.c_str()));
+    EXPECT_EQ(num_chars, buffer.size());
+  }
+};
+
+TEST_F(WriteIntoTest, WriteInto) {
   // Validate that WriteInto reserves enough space and
   // sizes a string correctly.
-  std::string buffer;
-  const char kOriginal[] = "supercali";
-  strncpy(WriteInto(&buffer, 1), kOriginal, 0);
-  EXPECT_STREQ("", buffer.c_str());
-  EXPECT_EQ(0u, buffer.size());
-  strncpy(WriteInto(&buffer, 2), kOriginal, 1);
-  EXPECT_STREQ("s", buffer.c_str());
-  EXPECT_EQ(1u, buffer.size());
-  strncpy(WriteInto(&buffer, 3), kOriginal, 2);
-  EXPECT_STREQ("su", buffer.c_str());
-  EXPECT_EQ(2u, buffer.size());
-  strncpy(WriteInto(&buffer, 5001), kOriginal, 5000);
-  EXPECT_STREQ("supercali", buffer.c_str());
-  EXPECT_EQ(5000u, buffer.size());
-  strncpy(WriteInto(&buffer, 3), kOriginal, 2);
-  EXPECT_STREQ("su", buffer.c_str());
-  EXPECT_EQ(2u, buffer.size());
-  strncpy(WriteInto(&buffer, 1), kOriginal, 0);
-  EXPECT_STREQ("", buffer.c_str());
-  EXPECT_EQ(0u, buffer.size());
-
-  // Validate that WriteInto returns NULL only when
-  // |length_with_null| == 1.
-  EXPECT_TRUE(WriteInto(&buffer, 1) == NULL);
-  EXPECT_TRUE(WriteInto(&buffer, 2) != NULL);
+  WritesCorrectly(1);
+  WritesCorrectly(2);
+  WritesCorrectly(5000);
 
   // Validate that WriteInto doesn't modify other strings
   // when using a Copy-on-Write implementation.
@@ -1149,9 +1142,9 @@ TEST(StringUtilTest, WriteInto) {
   const std::string live = kLive;
   std::string dead = live;
   strncpy(WriteInto(&dead, 5), kDead, 4);
-  EXPECT_STREQ(kDead, dead.c_str());
+  EXPECT_EQ(kDead, dead);
   EXPECT_EQ(4u, dead.size());
-  EXPECT_STREQ(kLive, live.c_str());
+  EXPECT_EQ(kLive, live);
   EXPECT_EQ(4u, live.size());
 }