Make string_util::WriteInto() DCHECK() that the supplied |length_with_null| > 1, meaning that the...

base/string_util.h

Index: base/string_util.h
===================================================================
--- base/string_util.h	(revision 107404)
+++ base/string_util.h	(working copy)
@@ -443,40 +443,32 @@
     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

[cbentzel, 2011/11/01 12:15:07]

I'm not sure about the fast comment.

With WriteInto, we do a zero-fill because of the resize(), followed by writing
directly into the buffer.

With a char* followed by Assign, we'd copy into the char* buffer followed by a
string copy.

These are likely about the same - but I haven't measured. The char* followed by
Assign may also have unneeded allocation and free for the char* [such as in a
scoped_array] which would be worse than WriteInto.

[Peter Kasting, 2011/11/01 19:43:28]

Mostly I was trying to restore some of the detail from the original comments on
this function.

I don't have violent feelings about the "fast" part, but I think at worst this
will be the same speed as the pattern you describe, and in some situations
better.  The real intent of the comment was more to indicate that we weren't
aware of an even better way to do this sort of access at the time we wrote the
function.

+// 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) {

[joth, 2011/11/01 11:25:32]

completely orthogonal issue to the one you're tackling here, but do the majority
of callers have to explicitly +1 to there argument? I'd find this API a lot more
intuitive if the size I pass in was the same as the size of the string after the
call, and hence the same as the number of bytes I am then permitted to write
into it.

The + 1 for null is really just an internal optimization, to remove the risk a
subsequent call to c_str() has to incur a reallocation. It doesn't seem to have
any bearing on the observable contract (to me).
If there's agreement on this I could look to do that change as a follow up...

Alternatively, for those callers that do not need to use c_str() and don't care
about trailing \0 (i.e. just dealing in raw binary data, not strings), we could
just change them to use resize() and string_as_array().

[Peter Kasting, 2011/11/01 19:43:28]

But the size of the string after the call and the number of bytes you are
permitted to write in are NOT the same.  And they can't be the same if you want
to support APIs that write a C-style string + trailing '\0' which was the
original point of WriteInto (since expanded to be used in more cases). 
Otherwise, callers either could not write the trailing byte, or they could write
it and it would be included in the string itself.
Not true, see above.

-  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;

[joth, 2011/11/01 11:25:32]

Worth noting there are a couple drawbacks in removing this:
- removes consistency with string_as_array (note that used to have non portable
optimizations for this case too, but I removed them as we care more for
portability than the origin  of that did)
- in the case someone does access the return value it turns a 'simple' NULL
pointer deref into a platform-dependent undefined behavior. (Of course, we hope
the DCHECK does catch it before that goes into a release)

[Peter Kasting, 2011/11/01 19:43:28]

And that would violate our "do not handle assertion failures" style rule.

-
   return &((*str)[0]);

[joth, 2011/11/01 11:25:32]

Maybe while you're here, the standards-approved way of doing this is:-
return &*str->begin();

see the comment on string_as_array in
http://src.chromium.org/viewvc/chrome/trunk/src/base/stl_util.h?revision=9335...
-- and also note the ISO/IEC 14882:2008 spec included the proposed wording about
this (see the second occurrence of 'contiguously' in
http://goto.google.com/twlli

[Peter Kasting, 2011/11/01 19:43:28]

I see the standard using this as an example of an identity that holds, but I
don't see it saying that what we're doing is in any sense wrong; in fact I read
the standard as guaranteeing that our form is equivalent.  See section 21.3.5
item 1 which says that operator[](pos) returns *(begin() + pos).

 }