[llvm] bbbc4f1 - Avoid keeping internal string_views in Twine.

[Sterling Augustine via llvm-commits]

Author: Sterling Augustine
Date: 2021-07-20T08:46:53-07:00
New Revision: bbbc4f110e35ac709b943efaa1c4c99ec073da30

URL: https://github.com/llvm/llvm-project/commit/bbbc4f110e35ac709b943efaa1c4c99ec073da30
DIFF: https://github.com/llvm/llvm-project/commit/bbbc4f110e35ac709b943efaa1c4c99ec073da30.diff

LOG: Avoid keeping internal string_views in Twine.

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 pointer-and-length at the cost of an extra eight-bytes
in Twine.

Differential Revision: https://reviews.llvm.org/D106186

Added: 
    

Modified: 
    llvm/include/llvm/ADT/Twine.h
    llvm/lib/Support/Twine.cpp
    llvm/unittests/ADT/TwineTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/ADT/Twine.h b/llvm/include/llvm/ADT/Twine.h
index 235b814f4d399..0885f9bb3593e 100644
--- a/llvm/include/llvm/ADT/Twine.h
+++ b/llvm/include/llvm/ADT/Twine.h
@@ -102,14 +102,14 @@ namespace llvm {
       /// 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,
 
@@ -145,11 +145,12 @@ namespace llvm {
     {
       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;
@@ -295,10 +296,14 @@ namespace llvm {
     }
 
 #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
@@ -430,11 +435,9 @@ namespace llvm {
       case EmptyKind:
       case CStringKind:
       case StdStringKind:
-#if __cplusplus > 201402L
-      case StdStringViewKind:
-#endif
       case StringRefKind:
       case SmallStringKind:
+      case PtrAndLengthKind:
         return true;
       default:
         return false;
@@ -469,14 +472,12 @@ namespace llvm {
         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);
       }
     }
 

diff  --git a/llvm/lib/Support/Twine.cpp b/llvm/lib/Support/Twine.cpp
index c1a254c99c700..5cbf8a06da539 100644
--- a/llvm/lib/Support/Twine.cpp
+++ b/llvm/lib/Support/Twine.cpp
@@ -65,14 +65,12 @@ void Twine::printOneChild(raw_ostream &OS, Child Ptr,
   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;
@@ -124,15 +122,14 @@ void Twine::printOneChildRepr(raw_ostream &OS, Child Ptr,
     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 << "\"";

diff  --git a/llvm/unittests/ADT/TwineTest.cpp b/llvm/unittests/ADT/TwineTest.cpp
index b9d107dea6772..e0fb7e55142e8 100644
--- a/llvm/unittests/ADT/TwineTest.cpp
+++ b/llvm/unittests/ADT/TwineTest.cpp
@@ -77,7 +77,7 @@ TEST(TwineTest, Concat) {
   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