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

[Justin Bogner via Phabricator via llvm-commits]

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D43901