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