<div class="gmail_quote">2011/5/20 Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:kyrtzidis@apple.com">kyrtzidis@apple.com</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word">(cc'ing cfe-commits)<div><br></div><div><div><div><div></div><div class="h5"><div>On May 20, 2011, at 11:04 AM, Matthieu Monrocq wrote:</div><br><blockquote type="cite">Hello,<br><br>
here is a second iteration of the patch<br><br><div class="gmail_quote">2011/5/19 Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>></span><br><blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex">

<div style="word-wrap:break-word"><div>(moved to cfe-commits)</div><div><br></div><div><blockquote type="cite"><div><font color="#000000">     /// Construct a string ref from a cstring.</font></div><div><font color="#000000">     /*implicit*/ StringRef(const char *Str)</font></div>

<div><font color="#000000">-      : Data(Str), Length(::strlen(Str)) {}</font></div><div><font color="#000000">+      : Data(Str), Length() {</font></div><div><font color="#000000">+        assert(Str && "StringRef cannot be built from a NULL argument");</font></div>

<div><font color="#000000">+        Length = ::strlen(Str); // invoking strlen(NULL) is undefined behavior</font></div><div><font color="#000000">+      }</font></div><div><font color="#000000"> </font></div></blockquote>

</div><div><div><br></div></div><div>"Length()" is not necessary.</div><div>Could you also add an assert in the  StringRef(const char *data, size_t length) constructor asserting that data is not null or length is 0 ?</div>

<div><br></div></div></blockquote><div><br>Removed and Done. <br></div><blockquote class="gmail_quote" style="margin-top:0pt;margin-right:0pt;margin-bottom:0pt;margin-left:0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204, 204, 204);padding-left:1ex">
<div style="word-wrap:break-word">
<div></div><div><blockquote type="cite"><div><font color="#000000">+    </font></div><div><font color="#000000">+    // Workaround memcmp issue with null pointers (undefined behavior)</font></div><div><font color="#000000">+    // by providing a specialized version</font></div>

<div><font color="#000000">+    static int memcmp(const char *Lhs, const char *Rhs, size_t Length) {</font></div><div><font color="#000000">+      if (Length == 0) { return 0; }</font></div><div><font color="#000000">+      assert(Lhs && "memcmp - Lhs should be non-null when Length is not 0");</font></div>

<div><font color="#000000">+      assert(Rhs && "memcmp - Rhs should be non-null when Length is not 0");</font></div><div><font color="#000000">+      return ::memcmp(Lhs,Rhs,Length);</font></div><div><font color="#000000">+    }</font></div>

<div><font color="#000000">+    </font></div></blockquote></div><div><div><br></div></div><div>Is this really necessary ? With the 2 asserts in the constructors we are making sure that StringRefs point to non-null or their length is zero, and calling memcmp with zero length is defined, no ?</div>

<div><br></div></div></blockquote><div> <br>I removed the two asserts since we now guarantee that Length is 0 if Data is null.<br><br>I am afraid the check might be necessary, from n869 (a Draft of C99)<br><br>> [7.21.1  String function conventions]<br>

> [#2] [...] Unless  explicitly  stated otherwise  in  the  description  of a particular function in this subclause, pointer arguments on such a call shall still have valid values, as described in 7.1.4. [...]<br><br>
[7.21.4.1  The memcmp function] does not state otherwise in any of its subclauses.<br>
<br>I've hit the bug on Suse with the memcpy function (on a memcpy(NULL, NULL, 0) call) and am now paranoid about it.<br></div></div></blockquote><div><br></div></div></div><div>Ugh, that is good know.</div><div>But could you please rename 'memcmp' to something else (e.g. 'compareMemory') ? I understand it was the choice with the least amount of changes, but it is confusing, in general, to have member functions with the same name as standard library functions.</div>
<div><br></div><div>-Argyrios</div></div></div></div></blockquote><div> </div><div>Done!<br><br>There were only 4 call sites so not too invasive.<br><br>-- Matthieu<br></div></div>