Explicit conversion of SmallString to std::string

[David Blaikie]

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/a0b648f5/attachment.html>