https://codereview.chromium.org/424973004/diff/1/include/v8-profiler.h
File include/v8-profiler.h (right):
https://codereview.chromium.org/424973004/diff/1/include/v8-profiler.h#newcode25
include/v8-profiler.h:25: unsigned int ticks;
hitCount?
https://codereview.chromium.org/424973004/diff/1/include/v8-profiler.h#newcode61
include/v8-profiler.h:61: * True if all available entries are copied,
otherwise false.
According to the implementation it copies nothing if buffer is not large
enough. I think it should be stated more clearly here.
https://codereview.chromium.org/424973004/diff/1/src/cpu-profiler.cc
File src/cpu-profiler.cc (left):
https://codereview.chromium.org/424973004/diff/1/src/cpu-profiler.cc#oldcode270
src/cpu-profiler.cc:270: ASSERT(Script::cast(shared->script()));
why is it gone?
https://codereview.chromium.org/424973004/diff/1/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):
https://codereview.chromium.org/424973004/diff/1/src/cpu-profiler.cc#newcode271
src/cpu-profiler.cc:271: if (position >= 0 && lineno > 0) {
if position < 0 you don't need to make a call to GetLineNumber
https://codereview.chromium.org/424973004/diff/1/src/log.h
File src/log.h (right):
https://codereview.chromium.org/424973004/diff/1/src/log.h#newcode163
src/log.h:163: if (0 == length) return 0;
nit style: the v8 code doesn't seem to use const at the left in
comparisons.
https://codereview.chromium.org/424973004/diff/1/src/log.h#newcode214
src/log.h:214: pc_offset_infos_.Add(pc_offset_info);
As long as the algo expects the entries are sorted by pc_offset, could
you please add the corresponding ASSERT here to be on the safe side?
https://codereview.chromium.org/424973004/diff/1/src/profile-generator-inl.h
File src/profile-generator-inl.h (right):
https://codereview.chromium.org/424973004/diff/1/src/profile-generator-inl.h#newcode31
src/profile-generator-inl.h:31: if (NULL != line_info) {
style nit: NULL to the right hand side
https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc
File src/profile-generator.cc (right):
https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#newcode216
src/profile-generator.cc:216: if (NULL != e) {
It couldn't be NULL if insert arg is true.
style: move const to the right hand side
https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#newcode217
src/profile-generator.cc:217: e->value = static_cast<char*>(e->value) +
1;
why char*? uintptr_t?
https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#newcode224
src/profile-generator.cc:224: if (NULL == entries || number == 0) {
ditto
https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#newcode229
src/profile-generator.cc:229: if (lineNumber == 0) return false;
should return true.
https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#newcode237
src/profile-generator.cc:237: entry->line =
reinterpret_cast<intptr_t>(p->key);
entry->line is unsigned int, i.e. 32 bits. So it should produce a
compile error on x64 (for sure on Win64).
https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#newcode635
src/profile-generator.cc:635: }
could you please make it more readable, e.g.:
int src_line = entry->line_info().GetSourceLineNumber(pc_offset);
if (src_line)
return src_line;
return v8::CpuProfileNode::kNoLineNumberInfo;
https://codereview.chromium.org/424973004/
--
--
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.
