llvm dev - Jul 2011 - [LLVMdev] Correct use of StringRef and Twine

On Jul 24, 2011, at 12:09 AM, David Blaikie wrote:
>> Yes, exactly.  I'm just saying that I think the additional clarity
of:
>>  "foo" + Twine('x')
>> 
>> is worth the inconvenience.
> 
> Ok, attached a modified version of my patch with an Twine(char),
> Twine(unsigned char), and Twine(signed char). All three are explicit &
> have included test cases.
Ok, great.
> Speaking of which - is there any LLVM dev policy or preference around
> C-style casts versus C++-style casts? Twine is littered with the
Nope, use the one that is the most clear in a given case.  Except in extreme
cases, the C++ style casts seem overly verbose and don't add a lot of value.
>>> As a side note on Twine's design: Is there a particular reason
it uses
>>> void*s rather than unions?
>> 
>> I'm not sure what you're proposing specifically.
> 
> I've attached an version of my fix that includes the union use I was
> referring to (sorry for being vague in the previous email). Though if
> we're considering a more explicitly structured solution with
> templates/subtypes/etc we could skip the whole mess of enums &
> unions/casts.
aha, that *is* much nicer!  Applied with a few tweaks (remove some extraneous
'explicit' keywords and wrapped to 80 cols).  Thanks!
>>> and chars rather than enums?
>> 
>> char vs enum is because of visual studio compatibility and because
enums often are stored as 32-bit values instead of 8-bit values.
> 
> Curious - seems VS >2k3 (
> http://msdn.microsoft.com/en-us/library/2dzy4k6e(v=VS.80).aspx ) has
> supported specifying enum backing type (so you can specify it as char:
> "enum foo : char { ... }") if that was the feature you were
wanting to
> use but avoiding.
Right, but that requires VS #ifdefs.  You can also use enum bitfields, but VS
has different promotion rules from them than other compilers.
>> Yes, from a general API design perspective, I hate having to force a
choice between "convenience to implementor of an API to just take a
StringRef" vs "convenience to client of API for it to take a
Twine".  It really stinks.
> 
> Ok - glad we're on the same page there, wasn't quite sure whether
we
> were talking past one another in previous emails.
> 
> I have a couple of ideas about how to lower the developer cost of
> using Twine ubiquitously. I'll try to describe them in some detail.
Ok!
>> I was chatting with Howard Hinnant about this and he suggested
replacing Twine with a template metaprogramming expression-template system.  I
haven't thought through all the details, but perhaps this would allow us to
get the best of both worlds?
> 
> I think I'd prefer a template metaprogramming solution to Twine just
> from a purist perspective compared to all the
> casting/reinterpreting/enums (even with the union) - that sort of
> stuff makes me a bit uncomfortable, but I realize it's well contained
> enough that it's not too risky (& I know some of the other tricks
LLVM
> uses like low bit twiddling in pointers, etc, so this seems relatively
> sane in the grand scheme of things).
Right.  As long as the damage is well contained and isolated in the class, I
don't care much about stuff like this.  One of the great features of C++ is
that it lets you do whatever insane thing you want and draw a nice interface
around it :)
> 1) the easy solution: create a StringRef subclass (or new type with a
> similarly expressive API) that wraps a buffer that it may or may not
> use. Give it a ctor that takes a Twine. The use case then goes from
> this:
> 
>    SmallVectorImpl<char> v;
>    StringRef s = theTwine.GetStringRef(v);
> 
> to this:
> 
>    TwineString s(theTwine);
> 
> (& the ctor looks like: TwineString(const Twine& t) { *this >
t.GetStringRef(this->buffer); })
> So any Twine sink now has to pay one line of code to get a usable
> string out of a Twine. Cheap to implement, easy to fix existing use
> cases (privatize GetStringRef(SmallVectorImpl<char>) and make
> TwineString a friend - fix everything that fails to compile).
> 
> This keeps things simple & seems to be "good enough" to me,
but we
> could perhaps do better (at the very least, again, if we did do
> better, we could go back & remove TwineString & again fix all the
> places that fail to compile with whatever new hotness we invent)
This definitely seems like a step forward.  While it is kinda gross, a subclass
of StringRef is probably the lowest friction path to do this.
> Hmm - new thought: what would happen if my argument type was simply
> TwineString? If I knew my function needed access to the values (like
> the Host.cpp example) but didn't have any buffer of its own that it
> needed to use, that seems OK. This new thought perhaps renders
> suggestion (2) uninteresting. Functions that know they always need the
> string contents take a TwineString. Functions that might be able to
> skip realizing the string entirely - or have their own buffer (eg: a
> member std::string) to populate - take a Twine & explicitly construct
> a TwineString only in the path that needs the value. That seems
> optimal to me.
I'm dubious of this.  This is exposing an implementation detail to clients
that they shouldn't have to know, and if passed by value, this could be
expensive.
> 1.1) We probably want a better way to get a Twine into a std::string -
> while the change to Triple's ctor works because returning a
> std::string from a function into a ctor can do fancy in-place
> construction - in any other case like "SetFoo(const Twine& t) {
> this->s = t; }" we'd want something better. Perhaps simply
> t.AssignTo(s);
Adding a Twine::assignTo method would make sense to me!
> 2) This one isn't quite as fleshed out, but see if the general outline
> makes sense: Introduce an extra type that's Twine-esque, but is only
> the top level (so it's implicitly constructible from a const
Twine&),
> rather than all the branches of a Twine. If a function wants to accept
> a Twine that it needs to manipulate, it can mark its argument as
> TwineString2 (for want of a better name, just using 2 to distinguish
> from the (1) example). TwineString2 has a buffer in it, but it starts
> uninitialized. Any attempt to retrieve/inspect values causes the
> string to be initialized. [downside: all string operations on a
> TwineString2 incur an "if not initialized" check]
> 
> Twines could be constructed from TwineString2s if the value needed to
> be passed on to another function. (& still take advantage of any
> initialized string buffer it might have, rather than having to
> remanifest from its child nodes repeatedly)
> 
> One fancy enhancement to this is that TwineString2 could partially
> manifest - eg: if you want ts[2] it could walk its children (dropping
> its references to children & updating the buffer with the contents at
> each step) until it found the 2nd character - then stop, with a
> partially expanded tree/buffer. Though there's probably nothing
> stopping us implementing such smarts in the (1) solution eventually
> either.
I think you're flying way over my head here :)

-Chris

> Right, but that requires VS #ifdefs.  You can also use enum bitfields, but
VS has different promotion rules from them than other compilers.
Ah, I'd thought that feature (specifying the backing type with "enum
name : integral_type") was standard, but I see it's a C++0x thing. My
mistake.
>> 1) the easy solution: create a StringRef subclass (or new type with a
>> similarly expressive API) that wraps a buffer that it may or may not
>> use. Give it a ctor that takes a Twine. The use case then goes from
>> this:
>>
>>    SmallVectorImpl<char> v;
>>    StringRef s = theTwine.GetStringRef(v);
>>
>> to this:
>>
>>    TwineString s(theTwine);
>>
>> (& the ctor looks like: TwineString(const Twine& t) { *this
>> t.GetStringRef(this->buffer); })
>> So any Twine sink now has to pay one line of code to get a usable
>> string out of a Twine. Cheap to implement, easy to fix existing use
>> cases (privatize GetStringRef(SmallVectorImpl<char>) and make
>> TwineString a friend - fix everything that fails to compile).
>>
>> This keeps things simple & seems to be "good enough" to
me, but we
>> could perhaps do better (at the very least, again, if we did do
>> better, we could go back & remove TwineString & again fix all
the
>> places that fail to compile with whatever new hotness we invent)
>
> This definitely seems like a step forward.  While it is kinda gross, a
subclass of StringRef is probably the lowest friction path to do this.
Come to think of it - is there any use for StringRef other than as
function arguments? If we get Twine to be 'better' for function
arguments than StringRef, then StringRef goes away - or put another
way: StringRef could be converted into TwineString (with or without a
rename) rather than deriving/duplicating the functionality into two
types.

It would be a matter of moving all of StringRef's ctors over to Twine,
though. At which point I'd be kind of inclined to start looking at a
templated solution to Twine since it'll have to start supporting
several more types (well, raw character arrays at least). I can do
that progressively - remove a StringRef ctor, add a Twine ctor, fix
all the places that took StringRefs that break because there's no
conversion anymore.

I'm going to have a play with this & see how it looks.
>> Hmm - new thought: what would happen if my argument type was simply
>> TwineString? If I knew my function needed access to the values (like
>> the Host.cpp example) but didn't have any buffer of its own that it
>> needed to use, that seems OK.
>
> I'm dubious of this.  This is exposing an implementation detail to
clients that they shouldn't have to know, and if passed by value, this could
be expensive.
Yeah, I'm on the fence about this. If we're OK with an extra line
(declaring the TwineString) for every string (Twine) argument, that
seems livable.
>> 2) This one isn't quite as fleshed out, but see if the general
outline
>> makes sense: Introduce an extra type that's Twine-esque, but is
only
>> the top level (so it's implicitly constructible from a const
Twine&),
>> rather than all the branches of a Twine. If a function wants to accept
>> a Twine that it needs to manipulate, it can mark its argument as
>> TwineString2 (for want of a better name, just using 2 to distinguish
>> from the (1) example). TwineString2 has a buffer in it, but it starts
>> uninitialized. Any attempt to retrieve/inspect values causes the
>> string to be initialized. [downside: all string operations on a
>> TwineString2 incur an "if not initialized" check]
>>
>> Twines could be constructed from TwineString2s if the value needed to
>> be passed on to another function. (& still take advantage of any
>> initialized string buffer it might have, rather than having to
>> remanifest from its child nodes repeatedly)
>>
>> One fancy enhancement to this is that TwineString2 could partially
>> manifest - eg: if you want ts[2] it could walk its children (dropping
>> its references to children & updating the buffer with the contents
at
>> each step) until it found the 2nd character - then stop, with a
>> partially expanded tree/buffer. Though there's probably nothing
>> stopping us implementing such smarts in the (1) solution eventually
>> either.
>
> I think you're flying way over my head here :)
No worries - I'm not sure it's really too compelling after (1),
really. Thin of (1) with the dangerous case (used as the argument
type), but with lazy evaluation. It has all the operations of
StringRef, like TwineString(1), but doesn't evaluate its
subexpressions (LHS/RHS in Twine) until an attempt is made to access
the string value. The last paragraph was an "ultra lazy" possibility -
where it could manifest part of the string (this would be harder to do
with template metaprogramming, I think - since it'd be more strictly
compile time). If you need the first character - well just manifest
the Twine up to the first non-empty branch, but there's no need to go
& create the whole string in a single buffer just yet.

I think this laziness would just be expensive though - involving lots
of checks to see if the string was manifest any time an access/string
operation was used.

- David

On Jul 24, 2011, at 6:31 PM, David Blaikie wrote:
>>> This keeps things simple & seems to be "good enough"
to me, but we
>>> could perhaps do better (at the very least, again, if we did do
>>> better, we could go back & remove TwineString & again fix
all the
>>> places that fail to compile with whatever new hotness we invent)
>> 
>> This definitely seems like a step forward.  While it is kinda gross, a
subclass of StringRef is probably the lowest friction path to do this.
> 
> Come to think of it - is there any use for StringRef other than as
> function arguments?
StringRef is useful for other things, for example the parsing logic in
ReadCheckFile in utils/FileCheck.cpp uses it effectively.  It really wants
"reference into another string" that is efficient to .substr() etc.
>  If we get Twine to be 'better' for function
> arguments than StringRef, then StringRef goes away - or put another
> way: StringRef could be converted into TwineString (with or without a
> rename) rather than deriving/duplicating the functionality into two
> types.
I think we still want StringRef as a simple "pointer+size tuple".
>>> Hmm - new thought: what would happen if my argument type was simply
>>> TwineString? If I knew my function needed access to the values
(like
>>> the Host.cpp example) but didn't have any buffer of its own
that it
>>> needed to use, that seems OK.
>> 
>> I'm dubious of this.  This is exposing an implementation detail to
clients that they shouldn't have to know, and if passed by value, this could
be expensive.
> 
> Yeah, I'm on the fence about this. If we're OK with an extra line
> (declaring the TwineString) for every string (Twine) argument, that
> seems livable.
Yeah, seems better than what we have today at least :)

-Chris