Explicit conversion of SmallString to std::string

[Yaron Keren]

One  more consideration, Twine.str() already returns std::string.

2014-10-15 19:18 GMT+03:00 David Blaikie <dblaikie at gmail.com>:

> Thanks for chiming in, Duncan. Mostly just agreeing with what you've said
> here.
>
> On Wed, Oct 15, 2014 at 8:59 AM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
>
>>
>> > On Oct 15, 2014, at 8:46 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> >
>> > On Wed, Oct 15, 2014 at 8:40 AM, Yaron Keren <yaron.keren at gmail.com>
>> wrote:
>> > SmallString has implicit conversion to StringRef:
>> >
>> >  operator StringRef() const { return str(); }
>>
>> IMO, this conversion operator should be be replaced with a constructor
>> in `StringRef`.
>>
>
> Are there any tradeoffs to consider between StringRef providing an
> implicit ctor from SmallString, compared to SmallString providing an
> implicit conversion to StringRef (which it already has)?
>
> What is possible with a StringRef(SmallString) ctor that's not possible
> (or convenient) with 'SmallString::operator StringRef'?
>
>
>>
>> >
>> > Explicit .str() helps when you want to use one of the StringRef useful
>> member functions without using an explicit StringRef temporary, such as:
>> >
>> >   SmallString<128> Name;
>> >   if (Name.str().startswith("blah"))
>> >
>> > Perhaps we could transform these instances to
>> StringRef(Name).startswith("blah") ? I'm not sure.
>> >
>>
>> I think David's right here.  This should just be `StringRef(Name).*`.
>>
>> Because of `StringRef::str()`, `Name.str()` looks like it's making a
>> string.  Consistent API is easier to think about.
>>
>> >
>> > StringRef does not have a constructor from SmallString probably to
>> avoid circular dependency.
>>
>> IMO the wrong tradeoff was made.
>>
>
> The only minor wrinkle is that this ctor will not be inline, because
> SmallString does really depend on StringRef (it has lots of functions that
> reasonably take StringRef by value, reference StringRef::npos (well, we
> could pull that out to some generic utility I suppose), etc) - so
> StringRef.h can have a forward declaration of SmallString which it can use
> to declare a StringRef(const SmallString&) ctor, then an out-of-line
> definition of that in StringRef.cpp.
>
>
>
>>
>> >
>> > BTW, .str() is already inconsistent since SmallString.str() is
>> StringRef but StringRef.str() is std::string.
>> >
>> > As for name, would SmallString.to_string() be better?
>> >
>> > Maybe (modulo our rather mixed attitudes towards naming conventions
>> (to_string, toString, etc)).
>> >
>> > Anyway - I just wanted to bring up the 'string' being a slightly
>> contentious name. I'd like some other voices on this thread & don't mean to
>> take "ownership" of the review here.
>>
>> I agree with David's original point here.  I think `.str()` should just
>> create a `std::string`.  I'd stage it like this:
>>
>>  1. Add a `StringRef::StringRef(SmallString)` constructor and remove
>>     the `SmallString::operator StringRef()` conversion.
>>
>>  2. Incrementally remove uses of `SmallString::str()`.
>>
>>  3. Remove `SmallString::str()` entirely once it's dead code.
>>
>>  4. After waiting a couple of days as a courtesy to be sure out-of-tree
>>     code has been updated, add back `SmallString::str()` with new
>>     semantics.
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141015/b0694e0b/attachment.html>