[llvm-commits] Twine/StringRef enhancements & usage

[David Blaikie]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110824/3d39e7cd/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: twine_partial.diff
Type: application/octet-stream
Size: 43369 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110824/3d39e7cd/attachment.obj>