Thanks for the new patch set. It's getting closer, but there are still a
handful
of mostly minor issues that you need to address.
https://codereview.chromium.org/12223027/diff/7001/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/12223027/diff/7001/include/v8.h#newcode3051
include/v8.h:3051: // RelocInfo::kNoPosition field.
This comment refers to internal implementation details
(RelocInfo::kNoPosition) in a public API. If you need to clarify the
semantic of this type, please describe it here completely rather than
referring to an internal implementation detail.
https://codereview.chromium.org/12223027/diff/7001/include/v8.h#newcode3066
include/v8.h:3066: // for CODE_ADD_LINE_POS_INFO and
CODE_END_LINE_INFO_RECORDING event
More explanation is needed here, since it's not just for "other events".
Right now, this is just used for *_LINE_INFO_* events, and the semantic
(the value returned from the START_LINE_INFO_RECORDING event in this
field is passed to subsequent CODE_START_LINE_INFO_RECORDING and
CODE_END_LINE_INFO_RECORDING events) should be captured in the
documentation.
https://codereview.chromium.org/12223027/diff/7001/src/assembler.cc
File src/assembler.cc (right):
https://codereview.chromium.org/12223027/diff/7001/src/assembler.cc#newcode1527
src/assembler.cc:1527: assembler_->pc_offset(),
why assembler_ rather than assembler()?
https://codereview.chromium.org/12223027/diff/7001/src/assembler.cc#newcode1544
src/assembler.cc:1544: assembler_->pc_offset(),
why assembler_ rather than assembler()?
https://codereview.chromium.org/12223027/diff/7001/src/assembler.h
File src/assembler.h (right):
https://codereview.chromium.org/12223027/diff/7001/src/assembler.h#newcode891
src/assembler.h:891: Assembler* assembler() {
Why is this necessary? You access directly assembler_ directly elsewhere
in this CL, and I don't see uses of assembler()?
https://codereview.chromium.org/12223027/diff/7001/src/log.cc
File src/log.cc (right):
https://codereview.chromium.org/12223027/diff/7001/src/log.cc#newcode479
src/log.cc:479: event.script =
v8::Handle<v8::Script>(reinterpret_cast<v8::Script*>(script));
Why don't you use "<Script>(ToApi<Script>(script))"? You will have to
pass in the script as an internal handle to use this, though.
https://codereview.chromium.org/12223027/diff/7001/src/log.cc#newcode534
src/log.cc:534: JitCodeEvent event;
How about making sure the event is a well-defined state before the call
(memset 0)? Or at fill it with kZapValue here and elsewhere when "#if
DEBUG" is true.
https://codereview.chromium.org/12223027/diff/7001/src/log.cc#newcode547
src/log.cc:547: code_event_handler_(&event);
same here
https://codereview.chromium.org/12223027/diff/7001/src/log.h
File src/log.h (right):
https://codereview.chromium.org/12223027/diff/7001/src/log.h#newcode250
src/log.h:250: // Emits a code line info add event with Postion type .
nit: remove space before .
https://codereview.chromium.org/12223027/diff/7001/src/log.h#newcode261
src/log.h:261: // It's the callee's responsibility to dispose the
parameter line_info data.
There is no "line_info data" parameter?
https://codereview.chromium.org/12223027/diff/7001/src/log.h#newcode381
src/log.h:381: Script* script,
Should be a Handle<Script>
https://codereview.chromium.org/12223027/diff/7001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):
https://codereview.chromium.org/12223027/diff/7001/test/cctest/test-api.cc#newcode11690
test/cctest/test-api.cc:11690: i::ComputePointerHash(line_info),
strange indentation, please follow style guidelines. The presubmit
script in the tools directory should find style problems like this one.
https://codereview.chromium.org/12223027/
--
--
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/groups/opt_out.
