[PATCH] A new ADT: StringRefNulTerminated

[David Blaikie]

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.


>
> > 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/73ff39fc/attachment.html>