[llvm-commits] Twine/StringRef enhancements & usage

[Michael Ilseman]

>> I don't get why you're using a SmallVectorImpl instead of a SmallVector or
>> SmallString, though.
>
> I think I just used SmallVectorImpl because it's the type that toStringRef
> required - but SmallString would make more sense if it's got no additional
> overhead/quirks (or SmallVector, presumably it really doesn't have extra
> overhead).
> Settled on SmallString.

I think one of the reasons that I've seen SmallVectorImpl instead of
SmallVector or SmallString for a function parameter is in order to
avoid having the template parameter that specifies initial size, which
you would have to do otherwise. For example, see
"FindFunctionBackedges" which can be declared in BasicBlockUtils.h and
defined in BasicBlockUtils.cpp without having template instantiation.
I don't know if this applies to your situation. As an aside, this
happens to unfortunately not be symmetric with SmallSetImpl vs
SmallSet where both take a size.

On Wed, Aug 24, 2011 at 10:40 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Wed, Aug 24, 2011 at 3:38 PM, Jordy Rose <jediknil at belkadan.com> wrote:
>>
>> A couple comments, though this certainly isn't an area of the code I'm
>> that familiar with.
>
> Thanks for looking
>
>>
>> - Instead of appendTo(string&) and assignTo(string&), why not just add
>> operator+=(string&, const Twine&) and operator=(string&, const Twine&) ?
>> Seems more C++ to me.
>
> You're right about appendTo, op+=(std::string&, const Twine&) could just be
> a friend function of Twine. I'll make that change.
> Unfortunately op=(std::string&, const Twine&) can't be done because op= must
> be a non-static member function (of std::string). This is why I hadn't done
> appendTo as op+= too - I'd assumed it had the same restriction, though that
> doesn't appear to be the case.
>
>>
>> - Re: toNullTerminatedStringRef: A StringRef created from a
>> null-terminated C string drops the null terminator, so you can't just "test
>> the last character" to see if it's a null. In fact, having the last
>> character of a StringRef be null is probably a bug. (Of course, you can't
>> test /past/ the last character either, because one byte past valid memory is
>> guaranteed to be a valid address but not guaranteed to be dereferenceable.)
>
> Agreed - pity, though. [I wonder if we could squeeze in a bit (the high bit
> of the length?) somewhere to store "is this null terminated" - it seems a
> pity to lose that so often/so easily when going into the StringRef domain]
>
>>
>> - You've got several copies of SafeBool.h in the file. I'm guessing this
>> is the result of reverting and then reapplying patches. (I do this all the
>> time too.)
>
> Hrm, thanks for that. I'll make a fresh diff. I've done that & manually
> inspected the diff file & I only see SafeBool.h listed once now.
>
>>
>> - TwineString definitely seems evil, but I haven't really thought about it
>> hard enough to give a good reason why.
>
> Oh, it is rather evil, just a moderately quick & dirty, but not utterly
> broken, solution. The most concrete reason I can come up with is that it
> muddies StringRef's semantics, mostly - TwineString is a StringRef, but it
> doesn't at all have the semantics of a StringRef, in fact it has the
> semantics of a string (mostly... some of the time... if it's not just
> actually a StringRef)
> To quote Chris from a previous email where this was discussed:
>
> "While it is kinda gross, a subclass of StringRef is probably the lowest
> friction path to do this." -
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2011-July/041804.html
> Not to say that I'm not open to other ideas...
>
>>
>> I don't get why you're using a SmallVectorImpl instead of a SmallVector or
>> SmallString, though.
>
> I think I just used SmallVectorImpl because it's the type that toStringRef
> required - but SmallString would make more sense if it's got no additional
> overhead/quirks (or SmallVector, presumably it really doesn't have extra
> overhead).
> Settled on SmallString.
> - David
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>