Avoid keeping internal string_views in Twine.

[saugustine]

Summary

This is a follow-up to https://reviews.llvm.org/D103935

A Twine's internal layout should not depend on which version of the
C++ standard is in use. Dynamically linking binaries compiled with two
different layouts (eg, --std=c++14 vs --std=c++17) ends up
problematic.

This change avoids that issue by immediately converting a
string_view to a StringRef at the cost of an extra eight-bytes
in Twine, and certain copies and dereferences to StringRefs that
hadn't happened prior to this change.

I do believe the optimizer can elide almost all the extra nominal
work via inlining and the like.

[saugustine]

tstellar:

Can you verify that this fixes your issue?

[dblaikie]

llvm/include/llvm/ADT/Twine.h

153–154

I think from a standards-conformance perspective this may need to be decomposed into a raw struct of const char*+length - otherwise, since StringRef isn't trivially constructible (guess that's why you had to add the Child ctor?) it won't be validly constructed and deconstructed as required by C++ for non-trivial types like this.

At least that's my vague/general understanding. There could be some nuance that makes this valid.

(as a follow-up, it'd be good to collapse all the other cases that can be represented by StringRef into this case to reduce the complexity of this code, I think)

164

If this does stay, the trailing semicolon should be removed

[saugustine]

This revision switches from a StringRef to an explicit pointer
and length.

[dblaikie]

Looks good to me, with the (ptr, length) ctor made private.

@dexonsmith could you double check this & see if it sounds reasonable?

llvm/include/llvm/ADT/Twine.h

304–308

I probably wouldn't add this ctor (if it's being added it should probably be unit tested - which I guess would be a perk of this design, that most of the string_view implementation would be unit testable even when string_view isn't available) - but if it's staging for common code, it could be added, but made private - then when other things (like StringRef) is changed over to use this ctor it'll get that generic (non-C++17-specific) test coverage.

[dexonsmith]

In D106186#2885180, @dblaikie wrote:

Looks good to me, with the (ptr, length) ctor made private.

@dexonsmith could you double check this & see if it sounds reasonable?

LGTM too.

[tstellar]

I've confirmed that this fixes my issue, thank you.

llvm/include/llvm/ADT/Twine.h

enum NodeKind : unsigned char {
       CStringKind,
 
       /// A pointer to an std::string instance.
       StdStringKind,
 
       /// A pointer to a StringRef instance.
       StringRefKind,
 
-#if __cplusplus > 201402L
-      // A pointer to a std::string_view instance.
-      StdStringViewKind,
-#endif
-
       /// A pointer to a SmallString instance.
       SmallStringKind,
 
+      /// A Pointer and Length representation. Used for std::string_view.
+      /// Can't use a StringRef here because they are not trivally
+      /// constructible.
+      PtrAndLengthKind,
+
       /// A pointer to a formatv_object_base instance.
       FormatvObjectKind,
 
       /// A char value, to render as a character.
       CharKind,
 
       /// An unsigned int value, to render as an unsigned decimal integer.
       DecUIKind,
enum NodeKind : unsigned char {
       /// integer.
       UHexKind
     };
 
     union Child
     {
       const Twine *twine;
       const char *cString;
+      struct {
+        const char *ptr;
+        size_t length;
+      } ptrAndLength;
       const std::string *stdString;
       const StringRef *stringRef;
-#if __cplusplus > 201402L
-      const std::string_view *stdStringView;
-#endif
       const SmallVectorImpl<char> *smallString;
       const formatv_object_base *formatvObject;
       char character;
       unsigned int decUI;
       int decI;
       const unsigned long *decUL;
       const long *decL;
       const unsigned long long *decULL;
       const long long *decLL;
       const uint64_t *uHex;
     };
 
     /// LHS - The prefix in the concatenation, which may be uninitialized for
     /// Null or Empty kinds.
     Child LHS;
 
     /// RHS - The suffix in the concatenation, which may be uninitialized for
     /// Null or Empty kinds.
     Child RHS;
public:
 
     /// Construct from an std::string.
     /*implicit*/ Twine(const std::string &Str) : LHSKind(StdStringKind) {
       LHS.stdString = &Str;
       assert(isValid() && "Invalid twine!");
     }
 
 #if __cplusplus > 201402L
-    /// Construct from an std::string_view.
+    /// Construct from an std::string_view by converting it to a pointer and
+    /// length.  This handles string_views on a pure API basis, and avoids
+    /// storing one (or a pointer to one) inside a Twine, which avoids problems
+    /// when mixing code compiled under various C++ standards.
     /*implicit*/ Twine(const std::string_view &Str)
-        : LHSKind(StdStringViewKind) {
-      LHS.stdStringView = &Str;
+        : LHSKind(PtrAndLengthKind) {
+      LHS.ptrAndLength.ptr = Str.data();
+      LHS.ptrAndLength.length = Str.length();
       assert(isValid() && "Invalid twine!");
     }
 #endif
 
     /// Construct from a StringRef.
     /*implicit*/ Twine(const StringRef &Str) : LHSKind(StringRefKind) {
       LHS.stringRef = &Str;
       assert(isValid() && "Invalid twine!");
     }
 
#endif
     /// StringRef value with getSingleStringRef().
     bool isSingleStringRef() const {
       if (getRHSKind() != EmptyKind) return false;
 
       switch (getLHSKind()) {
       case EmptyKind:
       case CStringKind:
       case StdStringKind:
-#if __cplusplus > 201402L
-      case StdStringViewKind:
-#endif
       case StringRefKind:
       case SmallStringKind:
+      case PtrAndLengthKind:
         return true;
       default:
         return false;
       }
     }
 
     /// @}
     /// @name String Operations
StringRef getSingleStringRef() const {
       switch (getLHSKind()) {
       default: llvm_unreachable("Out of sync with isSingleStringRef");
       case EmptyKind:
         return StringRef();
       case CStringKind:
         return StringRef(LHS.cString);
       case StdStringKind:
         return StringRef(*LHS.stdString);
-#if __cplusplus > 201402L
-      case StdStringViewKind:
-        return StringRef(*LHS.stdStringView);
-#endif
       case StringRefKind:
         return *LHS.stringRef;
       case SmallStringKind:
         return StringRef(LHS.smallString->data(), LHS.smallString->size());
+      case PtrAndLengthKind:
+        return StringRef(LHS.ptrAndLength.ptr, LHS.ptrAndLength.length);
       }
     }
 
     /// This returns the twine as a single StringRef if it can be
     /// represented as such. Otherwise the twine is written into the given
     /// SmallVector and a StringRef to the SmallVector's data is returned.
     StringRef toStringRef(SmallVectorImpl<char> &Out) const {
       if (isSingleStringRef())

llvm/lib/Support/Twine.cpp

void Twine::printOneChild(raw_ostream &OS, Child Ptr,
   case Twine::NullKind: break;
   case Twine::EmptyKind: break;
   case Twine::TwineKind:
     Ptr.twine->print(OS);
     break;
   case Twine::CStringKind:
     OS << Ptr.cString;
     break;
+  case Twine::PtrAndLengthKind:
+    OS << StringRef(Ptr.ptrAndLength.ptr, Ptr.ptrAndLength.length);
+    break;
   case Twine::StdStringKind:
     OS << *Ptr.stdString;
     break;
-#if __cplusplus > 201402L
-  case StdStringViewKind:
-    OS << StringRef(*Ptr.stdStringView);
-    break;
-#endif
   case Twine::StringRefKind:
     OS << *Ptr.stringRef;
     break;
   case Twine::SmallStringKind:
     OS << *Ptr.smallString;
     break;
   case Twine::FormatvObjectKind:
     OS << *Ptr.formatvObject;
void Twine::printOneChildRepr(raw_ostream &OS, Child Ptr,
   case Twine::TwineKind:
     OS << "rope:";
     Ptr.twine->printRepr(OS);
     break;
   case Twine::CStringKind:
     OS << "cstring:\""
        << Ptr.cString << "\"";
     break;
+  case Twine::PtrAndLengthKind:
+    OS << "ptrAndLength:\""
+       << StringRef(Ptr.ptrAndLength.ptr, Ptr.ptrAndLength.length) << "\"";
+    break;
   case Twine::StdStringKind:
     OS << "std::string:\""
        << Ptr.stdString << "\"";
     break;
-#if __cplusplus > 201402L
-  case Twine::StdStringViewKind:
-    OS << "std::string_view:\"" << StringRef(*Ptr.stdStringView) << "\"";
-    break;
-#endif
   case Twine::StringRefKind:
     OS << "stringref:\""
        << Ptr.stringRef << "\"";
     break;
   case Twine::SmallStringKind:
     OS << "smallstring:\"" << *Ptr.smallString << "\"";
     break;
   case Twine::FormatvObjectKind:

llvm/unittests/ADT/TwineTest.cpp

EXPECT_EQ("(Twine smallstring:\"hi\" empty)",
             repr(Twine().concat(Twine(SmallString<5>("hi")))));
   EXPECT_EQ("(Twine formatv:\"howdy\" empty)",
             repr(Twine(formatv("howdy")).concat(Twine())));
   EXPECT_EQ("(Twine formatv:\"howdy\" empty)",
             repr(Twine().concat(Twine(formatv("howdy")))));
   EXPECT_EQ("(Twine smallstring:\"hey\" cstring:\"there\")", 
             repr(Twine(SmallString<7>("hey")).concat(Twine("there"))));
 #if __cplusplus > 201402L
-  EXPECT_EQ("(Twine std::string_view:\"hey\" cstring:\"there\")",
+  EXPECT_EQ("(Twine ptrAndLength:\"hey\" cstring:\"there\")",
             repr(Twine(std::string_view("hey")).concat(Twine("there"))));
 #endif
 
   // Concatenation of unary ropes.
   EXPECT_EQ("(Twine cstring:\"a\" cstring:\"b\")", 
             repr(Twine("a").concat(Twine("b"))));
 
   // Concatenation of other ropes.