<div dir="rtl"><div dir="ltr"><a href="http://reviews.llvm.org/D6336" target="_blank">http://reviews.llvm.org/D6336</a><br></div><div dir="ltr">(Not ready yet)<br></div><div dir="ltr"><br></div><div class="gmail_extra" dir="ltr"><br><div class="gmail_quote"><div>2014-10-15 23:31 GMT+03:00 Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span>:</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>
> On Oct 15, 2014, at 9:18 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
><br>
> Thanks for chiming in, Duncan. Mostly just agreeing with what you've said here.<br>
<br>
<br>
</span><span>> 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>
<br>
> What is possible with a StringRef(SmallString) ctor that's not possible (or convenient) with 'SmallString::operator StringRef'?<br>
<br>
</span>Hmm... I think I've hit cases where implicit conversions cause ambiguity<br>
that implicit ctors don't, but I can't come up with an example right<br>
now.  Maybe that's just dogma from my past?<br>
<span><br>
> 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>
</span>We can add a constructor from `SmallVectorImpl<char>`, `SmallString`'s<br>
parent class.  This makes `StringRef` constructible from<br>
`SmallVector<char>` as well; I'm not sure whether that's a feature or a<br>
defect, but it seems reasonable to me.<br>
<br>
Or, we could use a cross-include:<br>
<br>
    $ cat StringRef.h<br>
    namespace llvm {<br>
    class StringRef {<br>
    public:<br>
      inline StringRef(const SmallString &S);<br>
      /* ... */<br>
    };<br>
    }<br>
    #include "llvm/ADT/SmallString.h"<br>
<br>
    $ cat SmallString.h<br>
    #include "llvm/Support/StringRef.h"<br>
    namespace llvm {<br>
    class SmallString { /* ... */ };<br>
    StringRef::StringRef(const SmallString &S) { /* ... */ }<br>
    }<br>
<br>
Or, if my aversion to implicit conversion operators is just religious,<br>
then we can just focus on fixing `SmallStr::str()` :).<br>
<div><div><br>
> 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>
</div></div></blockquote></div><br></div></div>