[PATCH] A new ADT: StringRefNulTerminated

[David Blaikie]

On Sat, Feb 23, 2013 at 3:51 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
>
> On Sat, Feb 23, 2013 at 2:56 PM, Dmitri Gribenko <gribozavr at gmail.com>wrote:
>
>> On Thu, Feb 21, 2013 at 2:43 AM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>> > On Wed, Feb 20, 2013 at 4:34 PM, Sean Silva <silvas at purdue.edu> wrote:
>> >>
>> >> Overall, I think this is a good idea to have.
>> >>
>> >> Some comments about the patch:
>> >>
>> >> +    /// Construct a string ref from a string literal.
>> >> +    template<size_t LengthWithNul>
>> >> +    StringRefNulTerminated(const char (&Str)[LengthWithNul]):
>> >> +      StringRef(Str, LengthWithNul - 1) {}
>> >>
>> >> Is there a threat that this could end up being called by a non-string
>> >> literal? If so, then it should assert. I'm thinking of a scenario like
>> >
>> >
>> > 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.
>>
>> The motivation for this constructor is a different.  I wanted to have
>> an implicit conversion from a string literal, but not from any 'const
>> char *' pointer, since it is not uncommon in LLVM to sometimes have a
>> 'const char *' that is not NUL-terminated -- that you just fetched
>> from a StringRef, or that would be put in a StringRef on the next
>> lines.
>>
>
> 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.
>

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?)


>
>
>>
>> > That would catch cases of large buffers with small null terminated
>> strings
>> > in them which the current patch doesn't handle correctly.
>>
>> That is an issue indeed.  But I don't think it is common.  What is your
>> opinion?
>>
>
> 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)
>
>
>>
>> >> const char Foo[1] = {'a'};
>> >> auto X = StringRefNulTerminated(Foo);
>> >
>> >
>> > 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.
>>
>> I added an assert anyway -- it might be helpful, and the compiler
>> should optimize it out in most cases anyway.
>>
>> Thank you for the review, I will post an updated patch in a few minutes.
>>
>> Dmitri
>>
>> --
>> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
>> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130223/98f49b33/attachment.html>