Make UMA_HISTOGRAM_ENUMERATION work with scoped enums.

base/metrics/histogram_macros_internal.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_METRICS_HISTOGRAM_MACROS_INTERNAL_H_
 #define BASE_METRICS_HISTOGRAM_MACROS_INTERNAL_H_
 
+#include <stdint.h>
+
+#include <limits>
+#include <type_traits>
+
 #include "base/atomicops.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/time/time.h"
 
 // This is for macros internal to base/metrics. They should not be used outside
 // of this directory. For writing to UMA histograms, see histogram_macros.h.
 
 // TODO(rkaplow): Improve commenting of these methods.
@@ skipping 71 matching lines @@
   } while (0)
 
 // This is a helper macro used by other macros and shouldn't be used directly.
 #define INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(name, sample, min, max,     \
                                                    bucket_count, flag)         \
     STATIC_HISTOGRAM_POINTER_BLOCK(                                            \
         name, Add(sample),                                                     \
         base::Histogram::FactoryGet(name, min, max, bucket_count, flag))
 
 // This is a helper macro used by other macros and shouldn't be used directly.
-// For an enumeration with N items, recording values in the range [0, N - 1],
-// this macro creates a linear histogram with N + 1 buckets:
-//   [0, 1), [1, 2), ..., [N - 1, N), and an overflow bucket [N, infinity).
+// The bucketing scheme is linear with a bucket size of 1. For N items,
+// recording values in the range [0, N - 1] creates a linear histogram with N +
+// 1 buckets:
+//   [0, 1), [1, 2), ..., [N - 1, N)
+// and an overflow bucket [N, infinity).
+//
 // Code should never emit to the overflow bucket; only to the other N buckets.
-// This allows future versions of Chrome to safely append new entries to the
-// enumeration. Otherwise, the histogram would have [N - 1, infinity) as its
-// overflow bucket, and so the maximal value (N - 1) would be emitted to this
-// overflow bucket. But, if an additional enumerated value were later added, the
-// bucket label for the value (N - 1) would change to [N - 1, N), which would
-// result in different versions of Chrome using different bucket labels for
-// identical data.
+// This allows future versions of Chrome to safely increase the boundary size.
+// Otherwise, the histogram would have [N - 1, infinity) as its overflow bucket,
+// and so the maximal value (N - 1) would be emitted to this overflow bucket.
+// But, if an additional value were later added, the bucket label for
+// the value (N - 1) would change to [N - 1, N), which would result in different
+// versions of Chrome using different bucket labels for identical data.
+#define INTERNAL_HISTOGRAM_EXACT_LINEAR_WITH_FLAG(name, sample, boundary,  \
+                                                  flag)                    \
+  do {                                                                     \
+    static_assert(!std::is_enum<decltype(sample)>::value,                  \
+                  "|sample| should not be an enum type!");                 \
+    static_assert(!std::is_enum<decltype(boundary)>::value,                \
+                  "|boundary| should not be an enum type!");               \
+    STATIC_HISTOGRAM_POINTER_BLOCK(                                        \
+        name, Add(sample),                                                 \
+        base::LinearHistogram::FactoryGet(name, 1, boundary, boundary + 1, \
+                                          flag));                          \
+  } while (0)
+
+// Similar to the previous macro but intended for enumerations. This delegates
+// the work to the previous macro, but supports scoped enumerations as well by
+// forcing an explicit cast to the HistogramBase::Sample integral type.
+//
+// Note the range checks verify two separate issues:
+// - that the declared enum max isn't out of range of HistogramBase::Sample
+// - that the declared enum max is > 0
+//
+// TODO(dcheng): This should assert that the passed in types are actually enum

[wychen, 2017/03/27 20:08:56]

Do we want to convert all UMA_HISTOGRAM_ENUMERATION() to
UMA_HISTOGRAM_EXACT_LINEAR() if either |sample| or |boundary| is not enum? Looks
like a good target for clang refactoring tools.

[dcheng, 2017/03/28 07:16:44]

Yes, that's the implication of this TODO. It's probably not even necessary for
tooling: we can just static assert that the input types are enum types, and feed
that to a simple script that changes the macro on that line.

+// types.
 #define INTERNAL_HISTOGRAM_ENUMERATION_WITH_FLAG(name, sample, boundary, flag) \
   do {                                                                         \
     static_assert(                                                             \
         !std::is_enum<decltype(sample)>::value ||                              \

[rkaplow, 2017/03/29 18:02:50]

since this macro should only be used for enums now (and not the linear model),
can't we now switch this check to verify both sample and boundary are enums
here? i.e. remove the first two !is_enum options.

[dcheng, 2017/03/29 18:24:40]

It should, but I'd prefer to do this in a followup (it's already a TODO on line
138): it's still possible to use non-enum things with this for the moment (if
you pass in a constexpr boundary).

             !std::is_enum<decltype(boundary)>::value ||                        \
             std::is_same<std::remove_const<decltype(sample)>::type,            \
                          std::remove_const<decltype(boundary)>::type>::value,  \
         "|sample| and |boundary| shouldn't be of different enums");            \
-    STATIC_HISTOGRAM_POINTER_BLOCK(                                            \
-        name, Add(sample), base::LinearHistogram::FactoryGet(                  \
-                               name, 1, boundary, boundary + 1, flag));        \
+    static_assert(                                                             \
+        static_cast<uintmax_t>(boundary) <                                     \
+            static_cast<uintmax_t>(                                            \
+                std::numeric_limits<base::HistogramBase::Sample>::max()),      \
+        "|boundary| is out of range of HistogramBase::Sample");                \
+    INTERNAL_HISTOGRAM_EXACT_LINEAR_WITH_FLAG(                                 \

[rkaplow, 2017/03/29 18:02:50]

From what I understand this macro forces the args to not be enums, which we're
doing by the casts here. Maybe worth adding a comment clarifying that?

[dcheng, 2017/03/29 18:24:40]

It's hard to add comments inside macros. Is there any particular comment you're
looking for other than the one already on line 130-132?

[rkaplow, 2017/03/29 23:21:17]

Yeah, I would prefer it down here but since it's not really possible with macros
I guess it's fine.

+        name, static_cast<base::HistogramBase::Sample>(sample),                \
+        static_cast<base::HistogramBase::Sample>(boundary), flag);             \
   } while (0)
 
 // This is a helper macro used by other macros and shouldn't be used directly.
 // This is necessary to expand __COUNTER__ to an actual value.
 #define INTERNAL_SCOPED_UMA_HISTOGRAM_TIMER_EXPANDER(name, is_long, key)       \
   INTERNAL_SCOPED_UMA_HISTOGRAM_TIMER_UNIQUE(name, is_long, key)
 
 // This is a helper macro used by other macros and shouldn't be used directly.
 #define INTERNAL_SCOPED_UMA_HISTOGRAM_TIMER_UNIQUE(name, is_long, key)         \
   class ScopedHistogramTimer##key {                                            \
@@ skipping 17 matching lines @@
 // may be more efficient in memory if the total number of sample values is small
 // compared to the range of their values.
 #define INTERNAL_HISTOGRAM_SPARSE_SLOWLY(name, sample)                         \
     do {                                                                       \
       base::HistogramBase* histogram = base::SparseHistogram::FactoryGet(      \
           name, base::HistogramBase::kUmaTargetedHistogramFlag);               \
       histogram->Add(sample);                                                  \
     } while (0)
 
 #endif  // BASE_METRICS_HISTOGRAM_MACROS_INTERNAL_H_