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

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/unittests/ADT/TwineTest.cpp:118
+  formatter::testNoFormat(formatv("{0}", Formatter));
+  EXPECT_EQ(0, Formatter.Count);
+  formatter::testWithFormat(formatv("{0}", Formatter));
----------------
dblaikie wrote:
> 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)
Yes the formatter gets copied in the formatv call.  Accessing the local count variable seems reasonable.




================
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);
----------------
dblaikie wrote:
> What's the difference between these two cases? Demonstrating that its reentrant/non-caching?
I suppose there isn't much.  I can remove the second one.


https://reviews.llvm.org/D27835