Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801613002:
Allow UmaHistogramEnumeration to be used with enum classes. (Closed)
DescriptionAllow UmaHistogramEnumeration to be used with enum classes.
This change allows the UmaHistogramEnumeration function to be used with
enum classes and not produce compile errors.
BUG=None
Review-Url: https://codereview.chromium.org/2801613002
Cr-Commit-Position: refs/heads/master@{#462401}
Committed: https://chromium.googlesource.com/chromium/src/+/1392ff9f43c86c539e53e698674693ef11c029da
Patch Set 1 #
Total comments: 2
Patch Set 2 : With DCHECKs #
Total comments: 4
Patch Set 3 : dcheng magic #Messages
Total messages: 27 (17 generated)
The CQ bit was checked by benwells@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
benwells@chromium.org changed reviewers: + dcheng@chromium.org, isherman@chromium.org
This is pulled out from https://codereview.chromium.org/2791193002/. Daniel - Ilya mentioned you are working on something related.
LGTM with a nit https://codereview.chromium.org/2801613002/diff/1/base/metrics/histogram_func... File base/metrics/histogram_functions.h (right): https://codereview.chromium.org/2801613002/diff/1/base/metrics/histogram_func... base/metrics/histogram_functions.h:46: return UmaHistogramExactLinear(name, static_cast<int>(sample), Nit: it might be nice to DCHECK that sample and max are in range of int.
LGTM, thanks.
The CQ bit was checked by benwells@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dcheng: I'd like feedback on how I fixed the nit, so please have another look. https://codereview.chromium.org/2801613002/diff/1/base/metrics/histogram_func... File base/metrics/histogram_functions.h (right): https://codereview.chromium.org/2801613002/diff/1/base/metrics/histogram_func... base/metrics/histogram_functions.h:46: return UmaHistogramExactLinear(name, static_cast<int>(sample), On 2017/04/05 17:12:13, dcheng wrote: > Nit: it might be nice to DCHECK that sample and max are in range of int. Hmmm .... OK. Not the simplest nit I've seen, or maybe I'm being dumb. https://codereview.chromium.org/2801613002/diff/20001/base/metrics/histogram_... File base/metrics/histogram_functions.h (right): https://codereview.chromium.org/2801613002/diff/20001/base/metrics/histogram_... base/metrics/histogram_functions.h:49: DCHECK_LE(sample, max); How is this? Some questions: - do you have a better way to do this? I tried doing things with static_assert but couldn't make it actually work. - I am pretty sure there aren't problems with this with numbers between INT64_MAX and UINT64_MAX, but tell me if I'm wrong. - Is it OK to do this in an inline function?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2801613002/diff/20001/base/metrics/histogram_... File base/metrics/histogram_functions.h (right): https://codereview.chromium.org/2801613002/diff/20001/base/metrics/histogram_... base/metrics/histogram_functions.h:49: DCHECK_LE(sample, max); On 2017/04/06 07:30:01, benwells wrote: > How is this? Some questions: > - do you have a better way to do this? I tried doing things with static_assert > but couldn't make it actually work. > - I am pretty sure there aren't problems with this with numbers between > INT64_MAX and UINT64_MAX, but tell me if I'm wrong. > - Is it OK to do this in an inline function? A smart person here suggested this: DCHECK_EQ(max, static_cast<T>(static_cast<int>(max))) WDYT?
On 2017/04/06 07:40:24, benwells wrote: > https://codereview.chromium.org/2801613002/diff/20001/base/metrics/histogram_... > File base/metrics/histogram_functions.h (right): > > https://codereview.chromium.org/2801613002/diff/20001/base/metrics/histogram_... > base/metrics/histogram_functions.h:49: DCHECK_LE(sample, max); > On 2017/04/06 07:30:01, benwells wrote: > > How is this? Some questions: > > - do you have a better way to do this? I tried doing things with static_assert > > but couldn't make it actually work. > > - I am pretty sure there aren't problems with this with numbers between > > INT64_MAX and UINT64_MAX, but tell me if I'm wrong. > > - Is it OK to do this in an inline function? > > A smart person here suggested this: > DCHECK_EQ(max, static_cast<T>(static_cast<int>(max))) > > WDYT? I think this might not work for values between INT_MAX and UINT_MAX: #include <stdint.h> int main() { constexpr uint32_t max = 0xffffffff; static_assert(max == static_cast<uint32_t>(static_cast<int>(max)), "Too big."); } This compiles successfully. https://codereview.chromium.org/2801613002/diff/20001/base/metrics/histogram_... File base/metrics/histogram_functions.h (right): https://codereview.chromium.org/2801613002/diff/20001/base/metrics/histogram_... base/metrics/histogram_functions.h:49: DCHECK_LE(sample, max); On 2017/04/06 07:30:01, benwells wrote: > How is this? Some questions: > - do you have a better way to do this? I tried doing things with static_assert > but couldn't make it actually work. Yeah, static assert won't work here. > - I am pretty sure there aren't problems with this with numbers between > INT64_MAX and UINT64_MAX, but tell me if I'm wrong. I think this is fine. I think the number of DCHECKs could probably be reduced though. DCHECK_LE(static_cast<uintmax_t>(max), static_cast<uintmax_t>(INT_MAX)); DCHECK_LE(static_cast<uintmax_t>(sample), static_cast<uintmax_t>(max)); which would cover the same thing (though it's slightly more subtle) > - Is it OK to do this in an inline function? Yeah, it's a DCHECK so it shouldn't* affect official builds. * of course it does sometimes. But we fixed all the ones that we can, and things like this hopefully aren't affected.
The CQ bit was checked by benwells@chromium.org to run a CQ dry run
https://codereview.chromium.org/2801613002/diff/20001/base/metrics/histogram_... File base/metrics/histogram_functions.h (right): https://codereview.chromium.org/2801613002/diff/20001/base/metrics/histogram_... base/metrics/histogram_functions.h:49: DCHECK_LE(sample, max); On 2017/04/06 07:47:05, dcheng wrote: > On 2017/04/06 07:30:01, benwells wrote: > > How is this? Some questions: > > - do you have a better way to do this? I tried doing things with static_assert > > but couldn't make it actually work. > > Yeah, static assert won't work here. > > > - I am pretty sure there aren't problems with this with numbers between > > INT64_MAX and UINT64_MAX, but tell me if I'm wrong. > > I think this is fine. I think the number of DCHECKs could probably be reduced > though. > DCHECK_LE(static_cast<uintmax_t>(max), static_cast<uintmax_t>(INT_MAX)); > DCHECK_LE(static_cast<uintmax_t>(sample), static_cast<uintmax_t>(max)); > > which would cover the same thing (though it's slightly more subtle) > > > - Is it OK to do this in an inline function? > > Yeah, it's a DCHECK so it shouldn't* affect official builds. > > * of course it does sometimes. But we fixed all the ones that we can, and things > like this hopefully aren't affected. OK ... yeah, subtle :) Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/06 08:16:48, benwells wrote: > https://codereview.chromium.org/2801613002/diff/20001/base/metrics/histogram_... > File base/metrics/histogram_functions.h (right): > > https://codereview.chromium.org/2801613002/diff/20001/base/metrics/histogram_... > base/metrics/histogram_functions.h:49: DCHECK_LE(sample, max); > On 2017/04/06 07:47:05, dcheng wrote: > > On 2017/04/06 07:30:01, benwells wrote: > > > How is this? Some questions: > > > - do you have a better way to do this? I tried doing things with > static_assert > > > but couldn't make it actually work. > > > > Yeah, static assert won't work here. > > > > > - I am pretty sure there aren't problems with this with numbers between > > > INT64_MAX and UINT64_MAX, but tell me if I'm wrong. > > > > I think this is fine. I think the number of DCHECKs could probably be reduced > > though. > > DCHECK_LE(static_cast<uintmax_t>(max), static_cast<uintmax_t>(INT_MAX)); > > DCHECK_LE(static_cast<uintmax_t>(sample), static_cast<uintmax_t>(max)); > > > > which would cover the same thing (though it's slightly more subtle) > > > > > - Is it OK to do this in an inline function? > > > > Yeah, it's a DCHECK so it shouldn't* affect official builds. > > > > * of course it does sometimes. But we fixed all the ones that we can, and > things > > like this hopefully aren't affected. > > OK ... yeah, subtle :) Thanks! LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by benwells@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2801613002/#ps40001 (title: "dcheng magic")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491470435212710,
"parent_rev": "cad69d6c427d2d0437b3a9461bee2dc702f9e569", "commit_rev":
"1392ff9f43c86c539e53e698674693ef11c029da"}
Message was sent while issue was closed.
Description was changed from ========== Allow UmaHistogramEnumeration to be used with enum classes. This change allows the UmaHistogramEnumeration function to be used with enum classes and not produce compile errors. BUG=None ========== to ========== Allow UmaHistogramEnumeration to be used with enum classes. This change allows the UmaHistogramEnumeration function to be used with enum classes and not produce compile errors. BUG=None Review-Url: https://codereview.chromium.org/2801613002 Cr-Commit-Position: refs/heads/master@{#462401} Committed: https://chromium.googlesource.com/chromium/src/+/1392ff9f43c86c539e53e6986746... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/1392ff9f43c86c539e53e6986746... |
