<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Feb 20, 2013 at 4:34 PM, Sean Silva <span dir="ltr"><<a href="mailto:silvas@purdue.edu" target="_blank">silvas@purdue.edu</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Overall, I think this is a good idea to have.<br>
<br>
Some comments about the patch:<br>
<br>
+    /// Construct a string ref from a string literal.<br>
+    template<size_t LengthWithNul><br>
+    StringRefNulTerminated(const char (&Str)[LengthWithNul]):<br>
+      StringRef(Str, LengthWithNul - 1) {}<br>
<br>
Is there a threat that this could end up being called by a non-string<br>
literal? If so, then it should assert. I'm thinking of a scenario like<br></blockquote><div><br></div><div style>As much fun as templated array ref ctors are - it's probably simpler just to take a const char*, use strlen always (you know it's null terminated - or at least the user is in the wrong if they pass you one that isn't) & let the compiler optimize away the strlen/inline the call when it's convenient to do so.</div>
<div style><br></div><div style>That would catch cases of large buffers with small null terminated strings in them which the current patch doesn't handle correctly.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
const char Foo[1] = {'a'};<br>
auto X = StringRefNulTerminated(Foo);<br></blockquote><div><br></div><div style>Like any c-string API, that's just UB. Sure, I suppose we could use the array-ref style thing to detect & assert on this, might be handy - but we don't do it for other cstring APIs so I don't see any particular reason to do it on this one.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+  /// \brief Represents a constant reference to a NUL-terminated string.<br>
+  /// It can be safely converted to 'const char *' without copying the<br>
+  /// underlying data.<br>
+  class StringRefNulTerminated : public StringRef {<br>
+  public:<br>
<br>
This comment should really say something explaining that "it is just a<br>
way to represent in the type system that 'one past the end' of the<br>
StringRef is valid memory guaranteed to be NUL". The discussion in the<br>
rst documentation could be significantly simplified by describing it<br>
like this as well.<br>
<br>
+  /// It can be safely converted to 'const char *' without copying the<br>
+  /// underlying data.<br>
<br>
You should make clear that this is the motivation e.g. "The motivation<br>
is that it can be safely ...". I would also drop "safely" to avoid<br>
even suggesting the idea of looking one-past-the-end of a regular<br>
StringRef.<br>
<br>
+    StringRefNulTerminated(const char *Data, size_t Length):<br>
+        StringRef(Data, Length) {<br>
+      assert(Data[Length] == '\0' && "");<br>
<br>
Please make the message actually helpful. When someone crashes on this<br>
assert they want something meaningful.<br>
<span class="HOEnZb"><font color="#888888"><br>
-- Sean Silva<br>
</font></span></blockquote></div><br></div></div>