<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Sat, Feb 23, 2013 at 3:51 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div class="im">On Sat, Feb 23, 2013 at 2:56 PM, Dmitri Gribenko <span dir="ltr"><<a href="mailto:gribozavr@gmail.com" target="_blank">gribozavr@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On Thu, Feb 21, 2013 at 2:43 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
> On Wed, Feb 20, 2013 at 4:34 PM, Sean Silva <<a href="mailto:silvas@purdue.edu" target="_blank">silvas@purdue.edu</a>> wrote:<br>
>><br>
>> 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>
><br>
><br>
> As much fun as templated array ref ctors are - it's probably simpler just to<br>
> take a const char*, use strlen always (you know it's null terminated - or at<br>
> least the user is in the wrong if they pass you one that isn't) & let the<br>
> compiler optimize away the strlen/inline the call when it's convenient to do<br>
> so.<br>
<br>
</div>The motivation for this constructor is a different. I wanted to have<br>
an implicit conversion from a string literal, but not from any 'const<br>
char *' pointer, since it is not uncommon in LLVM to sometimes have a<br>
'const char *' that is not NUL-terminated -- that you just fetched<br>
from a StringRef, or that would be put in a StringRef on the next<br>
lines.<br></blockquote><div><br></div></div><div>I don't follow - any user passing a char* to a function without a length alongside it knows that they'd have to be null terminated. StringRef(const char*) makes no more sense than StringRefNulTerminated(const char*) does it? Except the former "forgets" about the null terminator, the latter does not.</div>
</div></div></div></blockquote><div><br></div><div style>Could you provide an example of the bug someone could write if we had StringRefNulTerminated(const char*) that you couldn't write otherwise? (of course an API that needs to take a null terminated string that might not be a string literal would have to use a StringRef instead... which does have a (const char*) ctor - so where did that get us?)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
> That would catch cases of large buffers with small null terminated strings<br>
> in them which the current patch doesn't handle correctly.<br>
<br>
</div>That is an issue indeed. But I don't think it is common. What is your opinion?<br></blockquote><div><br></div></div><div>Not common, but no particular need to leave it as a problem (especially if we just switch to const char* rather than array/template based)</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
>> const char Foo[1] = {'a'};<br>
>> auto X = StringRefNulTerminated(Foo);<br>
><br>
><br>
> Like any c-string API, that's just UB. Sure, I suppose we could use the<br>
> array-ref style thing to detect & assert on this, might be handy - but we<br>
> don't do it for other cstring APIs so I don't see any particular reason to<br>
> do it on this one.<br>
<br>
</div>I added an assert anyway -- it might be helpful, and the compiler<br>
should optimize it out in most cases anyway.<br>
<br>
Thank you for the review, I will post an updated patch in a few minutes.<br>
<div><div><br>
Dmitri<br>
<br>
--<br>
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if<br>
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com" target="_blank">gribozavr@gmail.com</a>>*/<br>
</div></div></blockquote></div></div><br></div></div>
</blockquote></div><br></div></div>