[llvm-commits] Twine/StringRef enhancements & usage

[David Blaikie]

[+Chris, since he seemed to take an interest in the design discussion
surrounding this originally]

Bump/ping/etc.

This has been around for a few weeks now. Is this the sort of stuff
that I'd be better off getting getting commit access for & just having
it post-commit reviewed?

- David

On Wed, Aug 24, 2011 at 9: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
>