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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders 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) {
----------------
bogner wrote:
> 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.
I've moved it to llvm-config.h and made Statistics.h include it.

As for whether it belongs in llvm-config.h or Config.h. At the moment, we ship Statistics.h and it needs to have access to the macro so llvm-config.h makes sense right now.

I do want to look into making STATISTIC() more fine-grained though. I haven't thought much about how I'm going to achieve that but if I can move the #if into the cpp file instead then it will make more sense to use Config.h. That said, Statistic will still be public and anyone using it will be quite surprised if it doesn't count and they can't see that in the header.


================
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 {
----------------
bogner wrote:
> Why move this to the header / make this type public? AFAICT nobody uses it since the only interesting API for users is GetStatistics() below.
I'll move this back. An earlier version of the patch had GetStatistics() return this object itself


================
Comment at: include/llvm/ADT/StatisticInfo.h:62
+/// completes.
+const std::vector<std::pair<StringRef, unsigned>> GetStatistics();
+
----------------
bogner wrote:
> Don't capitalize function names. This should probably be getStatistics().
This one is capitalized to match the existing PrintStatistics() and PrintStatisticsJSON(). It would be strange to have one function in this family with a lower case first letter


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


Repository:
  rL LLVM

https://reviews.llvm.org/D43901