[LLVMdev] Correct use of StringRef and Twine

[David Blaikie]

> Are you concerned about the stack size of the smallvector's, or the amount of code that needs to be written?

Mostly the code that needs to be written (my suggestion of having
Twines be able to create mutable buffers internally would increase
stack size by even more than just one SmallVector per Twine sink, so I
haven't really been thinking stack space efficiency so much as
authoring efficiency). It would be nice if authors didn't have to
guess at how their callers might construct strings & could always
provide Twine argument types without making a tradeoff between
implementation simplicity/rapidity & runtime efficiency when consuming
such strings.

> The former is relying on how append works on std::string.  Beyond the number of bytes copied, the former could cause (depending on the implementation of std::string) a reallocation for every
append.

Ah, indeed - I can see how an implementation of std::string might not
want to do the same kind of growth mechanisms as is more common in,
say, std::vector, whereas Twine knows it's got to put a bunch of
things into the buffer so it can be designed with that in mind.

> Well written, the Twine version could just do a resize/reserve once, then fill in the bytes.  I don't know if Twine does that yet though.

Yep - something I'll keep in mind to take a look at.

> Right.  Something like this could work:
>
> foo(const Twine &T) {
> ...
>  TwineTmpBuffer Tmp;
>  StringSwitch(T.toString(Tmp)).....
>
> Which doesn't seem too horrible, just needs a typedef of smallvector to TwineTmpBuffer.

In a few choice places, maybe, but as the default way to pass string
parameters I think that'd be a hard sell as a general practice.

>> Another example of a string sink is, say, tools/llvmc/src/Hooks.cpp. I
>> found this while looking for uses of "+= '-'" to use the char support
>> for Twine I'd just added. But I can't upgrade the Arg argument from
>> const std::string& to const Twine& easily since it needs to actually
>> manipulate the string - find, access elements, and substr it.
>
> We should make ".str()" an efficient way to get a mutable std::string out.

In the case of Hooks.cpp I could do a similar transformation as the
one you suggested above, using the SmallVectorImpl - but my point is
that doing that to every string sink seems like it'd be a little
annoying. If you think it's suitable/the right thing I can do that &
if we find a terse/tidier/more streamlined way to handle string sinks
later we can always update this usage.

>> Should this work just be done case by case? If so, I don't think I'll
>> end up with much Twine usage, probably just a lot of StringRef usage &
>> lots of str() calls.
>
> Yes, I think it should be done carefully, one API at a time.  I think that would make sense.  Sticking with StringRef where it is possible is simple and good.

Hmm, this is one bit I'm not sure about. As I tried to explain above,
it seems problematic to have to choose your argument type on the basis
of how you think callers might use your API. From the perspective of a
caller, a Twine argument is at least as expressive as a StringRef
(since all StringRefs can be Twined implicitly), but it takes that
extra step to write the implementation.

Perhaps I'm aiming for some kind of purist/perfectionist argument that
isn't necessary or practical, but I hope I've been clear in explaining
my uncertainty/issue here.

Thanks,
- David