[PATCH] D27835: Add support to llvm::Twine for storing formatv objects

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/unittests/ADT/TwineTest.cpp:105
+TEST(TwineTest, LazyEvaluation) {
+  struct formatter : public FormatAdapter<int> {
+    explicit formatter(int &Count) : Count(Count), FormatAdapter(0) {}
----------------
public inheritance is the default for structs, so you can omit it if youl ike


================
Comment at: llvm/unittests/ADT/TwineTest.cpp:111-112
+
+    static void testNoFormat(const Twine &T) {}
+    static void testWithFormat(const Twine &T) { auto S = T.str(); }
+  };
----------------
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)


================
Comment at: llvm/unittests/ADT/TwineTest.cpp:118
+  formatter::testNoFormat(formatv("{0}", Formatter));
+  EXPECT_EQ(0, Formatter.Count);
+  formatter::testWithFormat(formatv("{0}", Formatter));
----------------
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)


================
Comment at: llvm/unittests/ADT/TwineTest.cpp:119-122
+  formatter::testWithFormat(formatv("{0}", Formatter));
+  EXPECT_EQ(1, Formatter.Count);
+  formatter::testWithFormat(formatv("{0}", Formatter));
+  EXPECT_EQ(2, Formatter.Count);
----------------
What's the difference between these two cases? Demonstrating that its reentrant/non-caching?


https://reviews.llvm.org/D27835