Skip to content

Commit

Permalink
Avoid keeping internal string_views in Twine.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Sterling-Augustine committed Jul 20, 2021
1 parent 84877a0 commit bbbc4f1
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 29 deletions.
37 changes: 19 additions & 18 deletions llvm/include/llvm/ADT/Twine.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}

Expand Down
17 changes: 7 additions & 10 deletions llvm/lib/Support/Twine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 << "\"";
Expand Down
2 changes: 1 addition & 1 deletion llvm/unittests/ADT/TwineTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit bbbc4f1

Please sign in to comment.