Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1725233002:
histogram_macros: Added some documentation explaining constant rule. (Closed)
|
Created:
4 years, 10 months ago by Matt Giuca Modified:
4 years, 8 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionhistogram_macros: Added some documentation explaining constant rule.
The |name| argument to the histogram macros must be a "runtime constant"
(it is the same value on all calls from each call site).
BUG=
Committed: https://crrev.com/d0e0172d381e8fa9f0aecb8276f19b8f019def6c
Cr-Commit-Position: refs/heads/master@{#385994}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Rename constant_name back to name. Revert formatting changes. #Patch Set 3 : Reword comment for clarity. #Messages
Total messages: 15 (6 generated)
mgiuca@chromium.org changed reviewers: + asvitkine@chromium.org, isherman@chromium.org
Follow-up to https://codereview.chromium.org/1721633002/ --- I was calling the macros with different names and it wasn't clear why I shouldn't do that. This should make it clearer. WDYT? Sorry about the huge reformat but a) almost every line overflowed 80, and b) I think the reformat looks nicer anyway.
Thanks! I agree that the formatting you chose is much more readable =) https://codereview.chromium.org/1725233002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/1725233002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:116: // All of these macros must be called with |constant_name| as a compile-time s/compile-time/runtime https://codereview.chromium.org/1725233002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:120: #define LOCAL_HISTOGRAM_TIMES(constant_name, sample) \ I'm not sure that constant_name is clearer here -- I don't think I'd really understand what it means, without the context of knowing how these macros work. IMO it would be better to just annotate each of the macros with a comment that documents that the |name| must be a runtime constant.
https://codereview.chromium.org/1725233002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/1725233002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:120: #define LOCAL_HISTOGRAM_TIMES(constant_name, sample) \ On 2016/02/24 06:47:02, Ilya Sherman wrote: > I'm not sure that constant_name is clearer here -- I don't think I'd really > understand what it means, without the context of knowing how these macros work. > IMO it would be better to just annotate each of the macros with a comment that > documents that the |name| must be a runtime constant. True. I debated whether to send this at all because I'm not sure I did it justice. OK, I'll rename it back (which means undoing all the clang-formatting too... not worth doing it without any other changes), and write some more docs instead.
Any updates on this CL? (Cleaning up my review queue.)
Sorry I left this. I reverted most of the changes. Now it is just a bit of documentation. It's hard to phrase this succinctly because it is so unusual (the concept of it having to be the same value from a given call site) but I think this is a good balance. PTAL. https://chromiumcodereview.appspot.com/1725233002/diff/1/base/metrics/histogr... File base/metrics/histogram_macros.h (right): https://chromiumcodereview.appspot.com/1725233002/diff/1/base/metrics/histogr... base/metrics/histogram_macros.h:116: // All of these macros must be called with |constant_name| as a compile-time On 2016/02/24 06:47:02, Ilya Sherman wrote: > s/compile-time/runtime Done. https://chromiumcodereview.appspot.com/1725233002/diff/1/base/metrics/histogr... base/metrics/histogram_macros.h:120: #define LOCAL_HISTOGRAM_TIMES(constant_name, sample) \ On 2016/02/24 07:22:14, Matt Giuca wrote: > On 2016/02/24 06:47:02, Ilya Sherman wrote: > > I'm not sure that constant_name is clearer here -- I don't think I'd really > > understand what it means, without the context of knowing how these macros > work. > > IMO it would be better to just annotate each of the macros with a comment that > > documents that the |name| must be a runtime constant. > > True. I debated whether to send this at all because I'm not sure I did it > justice. OK, I'll rename it back (which means undoing all the clang-formatting > too... not worth doing it without any other changes), and write some more docs > instead. Done.
Description was changed from ========== histogram_macros: Rename name to constant_name for clarity. Added some documentation explaining why this must be a constant. Run clang_format for good measure (was necessary on most lines anyway). ========== to ========== histogram_macros: Added some documentation explaining constant rule. The |name| argument to the histogram macros must be a "runtime constant" (it is the same value on all calls from each call site). BUG= ==========
Description was changed from ========== histogram_macros: Added some documentation explaining constant rule. The |name| argument to the histogram macros must be a "runtime constant" (it is the same value on all calls from each call site). BUG= ========== to ========== histogram_macros: Added some documentation explaining constant rule. The |name| argument to the histogram macros must be a "runtime constant" (it is the same value on all calls from each call site). BUG= ==========
LGTM, thanks.
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1725233002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1725233002/40001
Message was sent while issue was closed.
Description was changed from ========== histogram_macros: Added some documentation explaining constant rule. The |name| argument to the histogram macros must be a "runtime constant" (it is the same value on all calls from each call site). BUG= ========== to ========== histogram_macros: Added some documentation explaining constant rule. The |name| argument to the histogram macros must be a "runtime constant" (it is the same value on all calls from each call site). BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== histogram_macros: Added some documentation explaining constant rule. The |name| argument to the histogram macros must be a "runtime constant" (it is the same value on all calls from each call site). BUG= ========== to ========== histogram_macros: Added some documentation explaining constant rule. The |name| argument to the histogram macros must be a "runtime constant" (it is the same value on all calls from each call site). BUG= Committed: https://crrev.com/d0e0172d381e8fa9f0aecb8276f19b8f019def6c Cr-Commit-Position: refs/heads/master@{#385994} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d0e0172d381e8fa9f0aecb8276f19b8f019def6c Cr-Commit-Position: refs/heads/master@{#385994} |
