Here's a first round of comments.
https://codereview.chromium.org/11552033/diff/8002/include/v8.h File include/v8.h (left): https://codereview.chromium.org/11552033/diff/8002/include/v8.h#oldcode3024 include/v8.h:3024: nit: Please don't remove whitespace not in accordance with our coding style, which is 2 lines between declarations/functions/etc. https://codereview.chromium.org/11552033/diff/8002/include/v8.h File include/v8.h (right): https://codereview.chromium.org/11552033/diff/8002/include/v8.h#newcode3045 include/v8.h:3045: // It can be Script info for CODE_ADDDED event or line_info_struct How about removing the "It can be", and only two DDs in ADDED https://codereview.chromium.org/11552033/diff/8002/include/v8.h#newcode3047 include/v8.h:3047: void* userdata; user_data is probably better. Also, why not have two fields that are strongly typed for CODE_ADDED and CODE_ADD_LINE_POS_INFO/CODE_END_LINE_INFO_RECORDING rather than this single void*? https://codereview.chromium.org/11552033/diff/8002/include/v8.h#newcode3059 include/v8.h:3059: // Only valid for CODE_ADD_LINE_INFO CODE_ADD_LINE_POS_INFO https://codereview.chromium.org/11552033/diff/8002/include/v8.h#newcode3062 include/v8.h:3062: unsigned offset; Why use unsigned rather than a size_t? This seems more consistent with the rest of the API. https://codereview.chromium.org/11552033/diff/8002/include/v8.h#newcode3064 include/v8.h:3064: unsigned pos; Same here https://codereview.chromium.org/11552033/diff/8002/include/v8.h#newcode3065 include/v8.h:3065: // Whether this position is statement How about you clarify what it is if it isn't a statement, e.g. "Indicates whether this position is a statement (as opposed to a XXXX)" https://codereview.chromium.org/11552033/diff/8002/src/assembler.cc File src/assembler.cc (right): https://codereview.chromium.org/11552033/diff/8002/src/assembler.cc#newcode1518 src/assembler.cc:1518: jit_code_event_line_info_, nit: formatting according to google C++ style guide. https://codereview.chromium.org/11552033/diff/8002/src/assembler.cc#newcode1536 src/assembler.cc:1536: LOG_CODE_EVENT(this->assembler()->isolate(), CodeLinePosInfoAddEvent( nit: formatting according to google C++ style guide. https://codereview.chromium.org/11552033/diff/8002/src/assembler.h File src/assembler.h (left): https://codereview.chromium.org/11552033/diff/8002/src/assembler.h#oldcode56 src/assembler.h:56: nit: don't make whitespace changes unrelated to your CL. https://codereview.chromium.org/11552033/diff/8002/src/assembler.h#oldcode824 src/assembler.h:824: nit: don't make whitespace changes unrelated to your CL, also style guide is 2 lines between, so it's correct in it's original form. https://codereview.chromium.org/11552033/diff/8002/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/11552033/diff/8002/src/flag-definitions.h#newcode666 src/flag-definitions.h:666: DEFINE_bool(jit_profile, false, "profile the JITted code for extern profiling" I don't think you need this flag. Why not just make this functionality dependent on whether the jit handler has been installed or not? https://codereview.chromium.org/11552033/diff/8002/src/full-codegen.cc File src/full-codegen.cc (right): https://codereview.chromium.org/11552033/diff/8002/src/full-codegen.cc#newcode310 src/full-codegen.cc:310: if (FLAG_jit_profile) { See comment in flags-definition.h https://codereview.chromium.org/11552033/diff/8002/src/full-codegen.cc#newcode312 src/full-codegen.cc:312: InitializeJITLineInfo(); nit: indentation should be 4 chars. Better yet, break the line after the "=" and the rest of the statement will be on the same line. https://codereview.chromium.org/11552033/diff/8002/src/ia32/lithium-codegen-ia32.cc File src/ia32/lithium-codegen-ia32.cc (right): https://codereview.chromium.org/11552033/diff/8002/src/ia32/lithium-codegen-ia32.cc#newcode324 src/ia32/lithium-codegen-ia32.cc:324: if (instr->HasPointerMap()) { Why do you need to add this? I would think that this duplicates a lot of position information for instructions that already output position information, and don't understand the motivation behind the change. And if you do make this change, you need to make it for ARM as well. https://codereview.chromium.org/11552033/diff/8002/src/lithium.cc File src/lithium.cc (left): https://codereview.chromium.org/11552033/diff/8002/src/lithium.cc#oldcode438 src/lithium.cc:438: nit: unrelated whitespace change https://codereview.chromium.org/11552033/diff/8002/src/lithium.cc File src/lithium.cc (right): https://codereview.chromium.org/11552033/diff/8002/src/lithium.cc#newcode421 src/lithium.cc:421: InitializeJITLineInfo(); nit: indentation after line break, better yet break after "=" and indent line by 4. https://codereview.chromium.org/11552033/diff/8002/src/log.h File src/log.h (right): https://codereview.chromium.org/11552033/diff/8002/src/log.h#newcode257 src/log.h:257: // Emits a code line info finish record event Can you add comments about the memory management of the JITCodeLineInfo? After the detach isn't it the caller's responsibility to dispose the struct? https://codereview.chromium.org/11552033/diff/8002/src/log.h#newcode377 src/log.h:377: void IssueAddCodeLinePosInfoEvent(JITCodeLineInfo* lineinfo, why not just line_info? Easier to read. https://codereview.chromium.org/11552033/diff/8002/src/log.h#newcode381 src/log.h:381: void IssueStartCodePosInfoEvent(JITCodeLineInfo* lineinfo); why not just line_info? Easier to read. https://codereview.chromium.org/11552033/diff/8002/src/log.h#newcode382 src/log.h:382: void IssueEndCodePosInfoEvent(Code* code, void* lineinfostruct); why not just line_info? Easier to read. https://codereview.chromium.org/11552033/diff/8002/src/log.h#newcode545 src/log.h:545: : pc_(pc), pos_(pos), is_statement_(is_statement) { } nit: whitespace formatting needs to be according to style guide https://codereview.chromium.org/11552033/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
