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

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 16 09:44:24 PST 2016

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:




(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?


More information about the llvm-commits mailing list