Add support to llvm::Twine for storing formatv objects

[zturner]

Summary

Given a function foo(const Twine &T), it would be nice to be able to write foo(formatv("{0}{1}", x, y));. This patch enables that.

For some reason there's some formatting-only changes in here. I suspect this is due to an incompatibility between mono-repo and the git clang-format extension script. I'll try to clean these up before submitting.

[zturner]

Twine.h is all screwed up (upstream) so I'm going to have to figure this out and resubmit the patch so the formatting isn't messed up. Be back later with a fixed patch

[zturner]

Fixed patch, hopefully it's better this time.

[dblaikie]

Could you explain what makes each of the test cases interesting? (what they're testing that other test cases aren't) - it seems like a bit more testing than I'd expect, but I'm probably missing something about the mechanics of Twine.

Also - would it be worth adding a test case to demonstrate that it truly is lazy? If you introduce a type with a custom printer that has a side effect (such as modifying the object as it prints) - then create a Twine of a formatv, observe the side effect hasn't occurred, then stringify the Twine and observe that the side effect has occurred.

[zturner]

Removed a redundant test case and added a test case to verify laziness.

[dblaikie]

llvm/unittests/ADT/TwineTest.cpp

105

public inheritance is the default for structs, so you can omit it if youl ike

111–112

I'd probably skip these helper functions and the expressions below can just be:

Twine(formatv(...));

and

Twine(formatv(...)).str();

(possibly with (void) casts for documentation)

118

Either you can access the local (non-member) Count directly here, or you could probably simplify Formatter and just have it take no ctor argument and initialize the Count member to zero instead? (or is the Formatter copied into the formatv call? In which case I'd probably just access 'Count' directly as the local variable here)

119–122

What's the difference between these two cases? Demonstrating that its reentrant/non-caching?

[zturner]

llvm/unittests/ADT/TwineTest.cpp

118

Yes the formatter gets copied in the formatv call. Accessing the local count variable seems reasonable.

119–122

I suppose there isn't much. I can remove the second one.

llvm/include/llvm/ADT/Twine.h

 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <cassert>
 #include <cstdint>
 #include <string>
 
 namespace llvm {
 
+class formatv_object_base;
 class raw_ostream;
 
 /// Twine - A lightweight data structure for efficiently representing the
 /// concatenation of temporary values as strings.
 ///
 /// A Twine is a kind of rope, it represents a concatenated string using a
 /// binary-tree, where the string is the preorder of the nodes. Since the
 /// Twine can be efficiently rendered into a buffer when its result is used,
enum NodeKind : unsigned char {
     StdStringKind,
 
     /// A pointer to a StringRef instance.
     StringRefKind,
 
     /// A pointer to a SmallString instance.
     SmallStringKind,
 
+    /// 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,
 
     /// An int value, to render as a signed decimal integer.
     DecIKind,
class Twine {
   };
 
   union Child {
     const Twine *twine;
     const char *cString;
     const std::string *stdString;
     const StringRef *stringRef;
     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;
public:
 
   /// Construct from a SmallString.
   /*implicit*/ Twine(const SmallVectorImpl<char> &Str)
       : LHSKind(SmallStringKind), RHSKind(EmptyKind) {
     LHS.smallString = &Str;
     assert(isValid() && "Invalid twine!");
   }
 
+  /// Construct from a formatv_object_base.
+  /*implicit*/ Twine(const formatv_object_base &Fmt)
+      : LHSKind(FormatvObjectKind), RHSKind(EmptyKind) {
+    LHS.formatvObject = &Fmt;
+    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) : LHSKind(CharKind), RHSKind(EmptyKind) {
     LHS.character = static_cast<char>(Val);

llvm/lib/Support/Twine.cpp

 //===-- Twine.cpp - Fast Temporary String Concatenation -------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/Twine.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 
 std::string Twine::str() const {
   // If we're storing only a std::string, just return it.
   if (LHSKind == StdStringKind && RHSKind == EmptyKind)
     return *LHS.stdString;
 
+  // If we're storing a formatv_object, we can avoid an extra copy by formatting
+  // it immediately and returning the result.
+  if (LHSKind == FormatvObjectKind && RHSKind == EmptyKind)
+    return LHS.formatvObject->str();
+
   // Otherwise, flatten and copy the contents first.
   SmallString<256> Vec;
   return toStringRef(Vec).str();
 }
 
 void Twine::toVector(SmallVectorImpl<char> &Out) const {
   raw_svector_ostream OS(Out);
   print(OS);
case Twine::StdStringKind:
     OS << *Ptr.stdString;
     break;
   case Twine::StringRefKind:
     OS << *Ptr.stringRef;
     break;
   case Twine::SmallStringKind:
     OS << *Ptr.smallString;
     break;
+  case Twine::FormatvObjectKind:
+    OS << *Ptr.formatvObject;
+    break;
   case Twine::CharKind:
     OS << Ptr.character;
     break;
   case Twine::DecUIKind:
     OS << Ptr.decUI;
     break;
   case Twine::DecIKind:
     OS << Ptr.decI;
case Twine::StdStringKind:
     break;
   case Twine::StringRefKind:
     OS << "stringref:\""
        << Ptr.stringRef << "\"";
     break;
   case Twine::SmallStringKind:
     OS << "smallstring:\"" << *Ptr.smallString << "\"";
     break;
+  case Twine::FormatvObjectKind:
+    OS << "formatv:\"" << *Ptr.formatvObject << "\"";
+    break;
   case Twine::CharKind:
     OS << "char:\"" << Ptr.character << "\"";
     break;
   case Twine::DecUIKind:
     OS << "decUI:\"" << Ptr.decUI << "\"";
     break;
   case Twine::DecIKind:
     OS << "decI:\"" << Ptr.decI << "\"";

llvm/unittests/ADT/TwineTest.cpp

 //===- TwineTest.cpp - Twine unit tests -----------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
-#include "llvm/ADT/Twine.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Support/FormatAdapters.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 using namespace llvm;
 
 namespace {
 
 std::string repr(const Twine &Value) {
   std::string res;
   llvm::raw_string_ostream OS(res);
   Value.printRepr(OS);
   return OS.str();
 }
 
 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());
+  EXPECT_EQ("hi", Twine(formatv("{0}", "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) {
   
   // 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 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"))));
 
   // 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());
+  EXPECT_EQ(0, *Twine(formatv("{0}{1}", "how", "dy"))
+                    .toNullTerminatedStringRef(storage)
+                    .end());
+}
+
+TEST(TwineTest, LazyEvaluation) {
+  struct formatter : public FormatAdapter<int> {
+    explicit formatter(int &Count) : Count(Count), FormatAdapter(0) {}
+    int &Count;
+
+    void format(raw_ostream &OS, StringRef Style) { ++Count; }
+
+    static void testNoFormat(const Twine &T) {}
+    static void testWithFormat(const Twine &T) { auto S = T.str(); }
+  };
+
+  int Count = 0;
+  formatter Formatter(Count);
+  formatter::testNoFormat(formatv("{0}", Formatter));
+  EXPECT_EQ(0, Formatter.Count);
+  formatter::testWithFormat(formatv("{0}", Formatter));
+  EXPECT_EQ(1, Formatter.Count);
+  formatter::testWithFormat(formatv("{0}", Formatter));
+  EXPECT_EQ(2, Formatter.Count);
 }
 
   // 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