<div dir="rtl"><div dir="ltr">One  more consideration, Twine.str() already returns std::string.</div></div><div class="gmail_extra"><div dir="ltr"><br><div class="gmail_quote">2014-10-15 19:18 GMT+03:00 David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for chiming in, Duncan. Mostly just agreeing with what you've said here.<br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Wed, Oct 15, 2014 at 8:59 AM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span><br>
> On Oct 15, 2014, at 8:46 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Wed, Oct 15, 2014 at 8:40 AM, Yaron Keren <<a href="mailto:yaron.keren@gmail.com" target="_blank">yaron.keren@gmail.com</a>> wrote:<br>
> SmallString has implicit conversion to StringRef:<br>
><br>
>  operator StringRef() const { return str(); }<br>
<br>
</span>IMO, this conversion operator should be be replaced with a constructor<br>
in `StringRef`.<br></blockquote></span><div><br>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)?<br><br>What is possible with a StringRef(SmallString) ctor that's not possible (or convenient) with 'SmallString::operator StringRef'?<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><br>
><br>
> Explicit .str() helps when you want to use one of the StringRef useful member functions without using an explicit StringRef temporary, such as:<br>
><br>
>   SmallString<128> Name;<br>
>   if (Name.str().startswith("blah"))<br>
><br>
> Perhaps we could transform these instances to StringRef(Name).startswith("blah") ? I'm not sure.<br>
><br>
<br>
</span>I think David's right here.  This should just be `StringRef(Name).*`.<br>
<br>
Because of `StringRef::str()`, `Name.str()` looks like it's making a<br>
string.  Consistent API is easier to think about.<br>
<span><br>
><br>
> StringRef does not have a constructor from SmallString probably to avoid circular dependency.<br>
<br>
</span>IMO the wrong tradeoff was made.<br></blockquote><div><br></div></span><div>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.<br><br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><br>
><br>
> BTW, .str() is already inconsistent since SmallString.str() is StringRef but StringRef.str() is std::string.<br>
><br>
> As for name, would SmallString.to_string() be better?<br>
><br>
> Maybe (modulo our rather mixed attitudes towards naming conventions (to_string, toString, etc)).<br>
><br>
> 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.<br>
<br>
</span>I agree with David's original point here.  I think `.str()` should just<br>
create a `std::string`.  I'd stage it like this:<br>
<br>
 1. Add a `StringRef::StringRef(SmallString)` constructor and remove<br>
    the `SmallString::operator StringRef()` conversion.<br>
<br>
 2. Incrementally remove uses of `SmallString::str()`.<br>
<br>
 3. Remove `SmallString::str()` entirely once it's dead code.<br>
<br>
 4. After waiting a couple of days as a courtesy to be sure out-of-tree<br>
    code has been updated, add back `SmallString::str()` with new<br>
    semantics.<br>
<br>
</blockquote></span></div><br></div></blockquote></div></div></div>