[PATCH] D43901: Make STATISTIC() values available programmatically

Justin Bogner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 2 11:14:43 PST 2018

bogner added inline comments.

Comment at: include/llvm/ADT/Statistic.h:63
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_STATS)
+#if defined(LLVM_ENABLE_STATS)
    const Statistic &operator=(unsigned Val) {
This assumes that every file that includes llvm/ADT/Statistic.h included llvm/Config/config.h first, which is actually never true due to our rules about include order. OTOH, we can't include config.h in this header, because it's llvm internal. I think if we want to do this we need to publish LLVM_ENABLE_STATS into llvm-config.h as API.

Comment at: include/llvm/ADT/StatisticInfo.h:23-53
+namespace llvm {
+/// This class is used in a ManagedStatic so that it is created on demand (when
+/// the first statistic is bumped) and destroyed only when llvm_shutdown is
+/// called. We print statistics from the destructor.
+/// This class is also used to look up statistic values from applications that
+/// use LLVM.
+class StatisticInfo {
Why move this to the header / make this type public? AFAICT nobody uses it since the only interesting API for users is GetStatistics() below.

Comment at: include/llvm/ADT/StatisticInfo.h:62
+/// completes.
+const std::vector<std::pair<StringRef, unsigned>> GetStatistics();
Don't capitalize function names. This should probably be getStatistics().

Comment at: unittests/ADT/StatisticTest.cpp:28
+  Counter++;
+#if defined(LLVM_ENABLE_STATS)
+  EXPECT_EQ(Counter, 2u);
Related to the Config.h comment above, I'm pretty sure this is always false in the test, as is.



More information about the llvm-commits mailing list