Adding SmallString support to Twine

[yaron.keren]

Summary

Teach Twine to support SmallString to save on .str() calls and later remove str() usage for these frequent cases.
Broke from http://reviews.llvm.org/D6336 for easier review.

[yaron.keren]

Correct missing empty line.

[yaron.keren]

Rebased, added unit tests and some overloads to avoid ambiguity.

[chfast]

include/llvm/MC/MCContext.h

233–235

If Twine can handle SmallString now, wouldn't it be better to reduce the number of overloads in this case?

[yaron.keren]

include/llvm/MC/MCContext.h

233–235

The preferred function is GetOrCreateSymbol(StringRef) in which Name directly indexes Symbols[]. GetOrCreateSymbol(Twine) is a slower convenience function, using a temporary buffer to print out the Twine into a SmallString and then call the StringRef version.

With MakeArgString the (StringRef) function is virtual and all other versions defer work to it.
I did notice MakeArgString(std::string Str) should take Str by reference and will update this.

[yaron.keren]

Make MakeArgString and CopyString accept argument by reference.

[chfast]

lib/MC/MCContext.cpp

176–177

Do you think it is worth to check if Name isSingleStringRef?

[yaron.keren]

Implemented Paweł suggestion to use isSingleStringRef() to reduce the number of overloads. GetOrCreateSymbol, CopyString and MakeArgString accept only Twine now, virtual MakeArgString(StringRef) is still required and was renamed to MakeArgStringRef to avoid ambiguity with the Twine version.

isSingleStringRef() is used through the toStringRef(), which was inlined to give the compiler a chance at optimizing out the Twine construction-isSingleStringRef testing-getSingleStringRef code sequence for simple arguments.

Removed function names from Twine.h comments.

[chfast]

tools/clang/lib/Sema/CodeCompleteConsumer.cpp

254

I liked the argument name String better than T. I know the type has changed but CopyString copies strings.

[yaron.keren]

Updated CopyString argument name to 'String'.

[chfast]

LGTM

[yaron.keren]

Committed revision 232465.

include/llvm/ADT/Twine.h

 //===-- Twine.h - Fast Temporary String Concatenation -----------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_ADT_TWINE_H
 #define LLVM_ADT_TWINE_H
 
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DataTypes.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <cassert>
 #include <string>
 
 namespace llvm {
   template <typename T>
enum NodeKind : unsigned char {
       CStringKind,
 
       /// A pointer to an std::string instance.
       StdStringKind,
 
       /// A pointer to a StringRef instance.
       StringRefKind,
 
+      /// A pointer to a SmallString instance.
+      SmallStringKind,
+
       /// A char value reinterpreted as a pointer, to render as a character.
       CharKind,
 
       /// An unsigned int value reinterpreted as a pointer, to render as an
       /// unsigned decimal integer.
       DecUIKind,
 
       /// An int value reinterpreted as a pointer, to render as a signed
class Twine {
     };
 
     union Child
     {
       const Twine *twine;
       const char *cString;
       const std::string *stdString;
       const StringRef *stringRef;
+      const SmallVectorImpl<char> *smallString;
       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;
public:
 
     /// Construct from a StringRef.
     /*implicit*/ Twine(const StringRef &Str)
       : LHSKind(StringRefKind), RHSKind(EmptyKind) {
       LHS.stringRef = &Str;
       assert(isValid() && "Invalid twine!");
     }
 
+    /// Construct from a SmallString.
+    /*implicit*/ Twine(const SmallVectorImpl<char> &Str)
+      : LHSKind(SmallStringKind), RHSKind(EmptyKind) {
+      LHS.smallString = &Str;
+      assert(isValid() && "Invalid twine!");
+    }
+
     /// Construct from a char.
     explicit Twine(char Val)
       : LHSKind(CharKind), RHSKind(EmptyKind) {
       LHS.character = Val;
     }
 
     /// Construct from a signed char.
     explicit Twine(signed char Val)
public:
     bool isSingleStringRef() const {
       if (getRHSKind() != EmptyKind) return false;
 
       switch (getLHSKind()) {
       case EmptyKind:
       case CStringKind:
       case StdStringKind:
       case StringRefKind:
+      case SmallStringKind:
         return true;
       default:
         return false;
       }
     }
 
     /// @}
     /// @name String Operations
public:
     StringRef getSingleStringRef() const {
       assert(isSingleStringRef() &&"This cannot be had as a single stringref!");
       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);
       case StringRefKind:  return *LHS.stringRef;
+      case SmallStringKind:
+        return StringRef(LHS.smallString->data(), LHS.smallString->size());
       }
     }
 
     /// toStringRef - 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;
 

include/llvm/MC/MCContext.h

public:
     /// Create and return a directional local symbol for numbered label (used
     /// for "1b" or 1f" references).
     MCSymbol *GetDirectionalLocalSymbol(unsigned LocalLabelVal, bool Before);
 
     /// Lookup the symbol inside with the specified @p Name.  If it exists,
     /// return it.  If not, create a forward reference and return it.
     ///
     /// @param Name - The symbol name, which must be unique across all symbols.
     MCSymbol *GetOrCreateSymbol(StringRef Name);
+    MCSymbol *GetOrCreateSymbol(const SmallVectorImpl<char> &Name);
     MCSymbol *GetOrCreateSymbol(const Twine &Name);
 
     MCSymbol *getOrCreateSectionSymbol(const MCSectionELF &Section);
 
     MCSymbol *getOrCreateFrameAllocSymbol(StringRef FuncName, unsigned Idx);
 
     /// Get the symbol for \p Name, or null.
     MCSymbol *LookupSymbol(StringRef Name) const;
     MCSymbol *LookupSymbol(const Twine &Name) const;

include/llvm/Option/ArgList.h

public:
   /// @{
 
   /// MakeArgString - Construct a constant string pointer whose
   /// lifetime will match that of the ArgList.
   virtual const char *MakeArgString(StringRef Str) const = 0;
   const char *MakeArgString(const char *Str) const {
     return MakeArgString(StringRef(Str));
   }
-  const char *MakeArgString(std::string Str) const {
+  const char *MakeArgString(const SmallVectorImpl<char> &Str) const {
+    return MakeArgString(StringRef(Str.data(), Str.size()));
+  }
+  const char *MakeArgString(const std::string &Str) const {
     return MakeArgString(StringRef(Str));
   }
   const char *MakeArgString(const Twine &Str) const;
 
   /// \brief Create an arg string for (\p LHS + \p RHS), reusing the
   /// string at \p Index if possible.
   const char *GetOrMakeJoinedArgString(unsigned Index, StringRef LHS,
                                         StringRef RHS) const;

lib/MC/MCContext.cpp

 }
 
 MCSymbol *MCContext::createTempSymbol(const Twine &Name) {
   SmallString<128> NameSV;
   raw_svector_ostream(NameSV) << MAI->getPrivateGlobalPrefix() << Name;
   return CreateSymbol(NameSV);
 }
 
+MCSymbol *MCContext::GetOrCreateSymbol(const SmallVectorImpl<char> &Name) {
+  return GetOrCreateSymbol(StringRef(Name.data(), Name.size()));
+}
+
 MCSymbol *MCContext::GetOrCreateSymbol(const Twine &Name) {
   SmallString<128> NameSV;
   return GetOrCreateSymbol(Name.toStringRef(NameSV));
 }
 
 MCSymbol *MCContext::CreateLinkerPrivateTempSymbol() {
   SmallString<128> NameSV;
   raw_svector_ostream(NameSV)
     << MAI->getLinkerPrivateGlobalPrefix() << "tmp" << NextUniqueID++;
   return CreateSymbol(NameSV);
 }

lib/Support/Twine.cpp

case Twine::CStringKind:
     OS << Ptr.cString;
     break;
   case Twine::StdStringKind:
     OS << *Ptr.stdString;
     break;
   case Twine::StringRefKind:
     OS << *Ptr.stringRef;
     break;
+  case Twine::SmallStringKind:
+    OS << *Ptr.smallString;
+    break;
   case Twine::CharKind:
     OS << Ptr.character;
     break;
   case Twine::DecUIKind:
     OS << Ptr.decUI;
     break;
   case Twine::DecIKind:
     OS << Ptr.decI;
void Twine::printOneChildRepr(raw_ostream &OS, Child Ptr,
   case Twine::StdStringKind:
     OS << "std::string:\""
        << Ptr.stdString << "\"";
     break;
   case Twine::StringRefKind:
     OS << "stringref:\""
        << Ptr.stringRef << "\"";
     break;
+  case Twine::SmallStringKind:
+    OS << "smallstring:\""
+       << *Ptr.smallString << "\"";
+    break;
   case Twine::CharKind:
     OS << "char:\"" << Ptr.character << "\"";
     break;
   case Twine::DecUIKind:
     OS << "decUI:\"" << Ptr.decUI << "\"";
     break;
   case Twine::DecIKind:
     OS << "decI:\"" << Ptr.decI << "\"";

tools/clang/include/clang/Sema/CodeCompleteConsumer.h

 
 /// \brief An allocator used specifically for the purpose of code completion.
 class CodeCompletionAllocator : public llvm::BumpPtrAllocator {
 public:
   /// \brief Copy the given string into this allocator.
   const char *CopyString(StringRef String);
 
   /// \brief Copy the given string into this allocator.
-  const char *CopyString(Twine String);
+  const char *CopyString(const Twine &String);
 
   // \brief Copy the given string into this allocator.
   const char *CopyString(const char *String) {
     return CopyString(StringRef(String));
   }
 
+  // \brief Copy the given string into this allocator.
+  const char *CopyString(const SmallVectorImpl<char> &String) {
+    return CopyString(StringRef(String.data(), String.size()));
+  }
+
   /// \brief Copy the given string into this allocator.
   const char *CopyString(const std::string &String) {
     return CopyString(StringRef(String));
   }
 };
 
 /// \brief Allocator for a cached set of global code completions.
 class GlobalCodeCompletionAllocator 

tools/clang/lib/Sema/CodeCompleteConsumer.cpp

 const char *CodeCompletionString::getTypedText() const {
   for (iterator C = begin(), CEnd = end(); C != CEnd; ++C)
     if (C->Kind == CK_TypedText)
       return C->Text;
 
   return nullptr;
 }
 
 const char *CodeCompletionAllocator::CopyString(StringRef String) {
   char *Mem = (char *)Allocate(String.size() + 1, 1);
   std::copy(String.begin(), String.end(), Mem);
   Mem[String.size()] = 0;
   return Mem;
 }
 
-const char *CodeCompletionAllocator::CopyString(Twine String) {
+const char *CodeCompletionAllocator::CopyString(const Twine &String) {
   // FIXME: It would be more efficient to teach Twine to tell us its size and
   // then add a routine there to fill in an allocated char* with the contents
   // of the string.
   SmallString<128> Data;
   return CopyString(String.toStringRef(Data));
 }
 
 StringRef CodeCompletionTUInfo::getParentName(const DeclContext *DC) {

unittests/ADT/TwineTest.cpp

 
 TEST(TwineTest, Construction) {
   EXPECT_EQ("", Twine().str());
   EXPECT_EQ("hi", Twine("hi").str());
   EXPECT_EQ("hi", Twine(std::string("hi")).str());
   EXPECT_EQ("hi", Twine(StringRef("hi")).str());
   EXPECT_EQ("hi", Twine(StringRef(std::string("hi"))).str());
   EXPECT_EQ("hi", Twine(StringRef("hithere", 2)).str());
+  EXPECT_EQ("hi", Twine(SmallString<4>("hi")).str());
 }
 
 TEST(TwineTest, Numbers) {
   EXPECT_EQ("123", Twine(123U).str());
   EXPECT_EQ("123", Twine(123).str());
   EXPECT_EQ("-123", Twine(-123).str());
   EXPECT_EQ("123", Twine(123).str());
   EXPECT_EQ("-123", Twine(-123).str());
TEST(TwineTest, Concat) {
   EXPECT_EQ("(Twine null empty)", 
             repr(Twine::createNull().concat(Twine("hi"))));
   
   // Concat with empty.
   EXPECT_EQ("(Twine cstring:\"hi\" empty)", 
             repr(Twine("hi").concat(Twine())));
   EXPECT_EQ("(Twine cstring:\"hi\" empty)", 
             repr(Twine().concat(Twine("hi"))));
+  EXPECT_EQ("(Twine smallstring:\"hi\" empty)", 
+            repr(Twine().concat(Twine(SmallString<5>("hi")))));
+  EXPECT_EQ("(Twine smallstring:\"hey\" cstring:\"there\")", 
+            repr(Twine(SmallString<7>("hey")).concat(Twine("there"))));
 
   // Concatenation of unary ropes.
   EXPECT_EQ("(Twine cstring:\"a\" cstring:\"b\")", 
             repr(Twine("a").concat(Twine("b"))));
 
   // Concatenation of other ropes.
   EXPECT_EQ("(Twine rope:(Twine cstring:\"a\" cstring:\"b\") cstring:\"c\")", 
             repr(Twine("a").concat(Twine("b")).concat(Twine("c"))));
   EXPECT_EQ("(Twine cstring:\"a\" rope:(Twine cstring:\"b\" cstring:\"c\"))",
             repr(Twine("a").concat(Twine("b").concat(Twine("c")))));
+  EXPECT_EQ("(Twine cstring:\"a\" rope:(Twine smallstring:\"b\" cstring:\"c\"))",
+            repr(Twine("a").concat(Twine(SmallString<3>("b")).concat(Twine("c")))));
 }
 
 TEST(TwineTest, toNullTerminatedStringRef) {
   SmallString<8> storage;
   EXPECT_EQ(0, *Twine("hello").toNullTerminatedStringRef(storage).end());
   EXPECT_EQ(0,
            *Twine(StringRef("hello")).toNullTerminatedStringRef(storage).end());
+  EXPECT_EQ(0, *Twine(SmallString<11>("hello"))
+                    .toNullTerminatedStringRef(storage)
+                    .end());
 }
 
   // I suppose linking in the entire code generator to add a unit test to check
   // the code size of the concat operation is overkill... :)
 
 } // end anonymous namespace