Explicit conversion of SmallString to std::string

Duncan P. N. Exon Smith dexonsmith at apple.com
Wed Oct 15 08:59:07 PDT 2014


> 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`.

> 
> 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.

> 
> 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.





More information about the llvm-commits mailing list