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
+// types.

[Ilya Sherman, 2017/03/31 00:28:14]

It looks to me like the current code allows, say, doubles to be passed in as
arguments to this macro.  Is that working as intended?

[dcheng, 2017/03/31 00:46:00]

I'll be addressing this TODO shortly. I expect to find interesting code in the
process, but the end state should be relatively type safe (well, as safe as a
macro is going to be =)

[Ilya Sherman, 2017/03/31 00:47:15]

Okay, lovely -- thanks!

 #define INTERNAL_HISTOGRAM_ENUMERATION_WITH_FLAG(name, sample, boundary, flag) \
   do {                                                                         \
     static_assert(                                                             \
         !std::is_enum<decltype(sample)>::value ||                              \
             !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(                                 \
+        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_