[PATCH] D49560: Add support to track total counts for DebugCounter

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added inline comments.


================
Comment at: include/llvm/Support/DebugCounter.h:162
+  };
+  DenseMap<unsigned, CounterInfo> Counters;
   DenseMap<unsigned, std::string> CounterDesc;
----------------
zhizhouy wrote:
> george.burgess.iv wrote:
> > I don't know the history of this code, but it seems reasonable to me to make this a `vector`, add a `string Desc` to `CounterInfo`, and drop `CounterDesc` entirely. `CounterVector` just returns `[1..UINT_MAX]`, in order, on insertion anyway.
> > 
> > (Alternatively, it seems that we could replace all three of these containers with a single `StringMap<CounterInfo>` and hand `CounterInfo *`s out instead of `unsigned`s, but that starts to stray pretty far from the goal of this patch, so... :) )
> I have moved Desc into the CounterInfo struct. Using vector to replace DenseMap may cause some ambiguity from UniqueVector on index. So I am keeping the DenseMap for now. We may come up with another patch to do the second change you mentioned.
This WFM, but I don't quite understand what you mean by

> Using vector to replace DenseMap may cause some ambiguity from UniqueVector on index

For my curiosity, can you elaborate a bit, please? :)


================
Comment at: unittests/Support/DebugCounterTest.cpp:1
+//===- llvm/unittest/Support/DebugCounterTest.cpp --------------------------------===//
+//
----------------
Is this 80 column aligned?


================
Comment at: unittests/Support/DebugCounterTest.cpp:16
+
+DEBUG_COUNTER(TestCounter, "test-counter",
+              "Counter used for unit test");
----------------
We probably want to put this under the `NDEBUG` guard, as well.

Otherwise, we'll have an unused static, which will probably cause compiler warnings.


Repository:
  rL LLVM

https://reviews.llvm.org/D49560