lgtm
Assuming the '10s' thing in counters.xx is fixed. The other two comments are
just nitpicks.
https://codereview.chromium.org/875873002/diff/1/src/counters.cc
File src/counters.cc (right):
https://codereview.chromium.org/875873002/diff/1/src/counters.cc#newcode70
src/counters.cc:70: name##_ = AggregatableHistogramTimer(#caption, 0,
1000000, 50, isolate);
One more '0' please. Compile times in single digit seconds aren't
unheard of, so I think the counter should go to 10s, as it previously
did.
https://codereview.chromium.org/875873002/diff/1/src/counters.h
File src/counters.h (right):
https://codereview.chromium.org/875873002/diff/1/src/counters.h#newcode364
src/counters.h:364: #define MILLISECONDS *1000
I'm not super fond of this style of macro meta programming, but I guess
this is personal preference. I think I'd prefer either plain const
expressions for the conversion factors or at least macros with
parameters (that is, SECONDS(10)).
Not a big deal though, so please decide as you see fit.
https://codereview.chromium.org/875873002/diff/1/src/counters.h#newcode728
src/counters.h:728: stats_counter_count
This (and above) is a super strange indent.
I take it this is clang-format. The same occured on my recent CL in the
file, and I just reverted these (local) changes and rather ignored
clang-format than mess up the code.
Not sure if there's a policy for this.
https://codereview.chromium.org/875873002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
