http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc File src/gdbjit.cc (right):
http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode95 src/gdbjit.cc:95: Slot<T> SlotHere() { The name should reflect the fact that this function (and the next) create slots. eg CreateSlotHere() http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode102 src/gdbjit.cc:102: Ensure(position_ += sizeof(T) * count); no no no no http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode311 src/gdbjit.cc:311: WriteString(""); I'm sure there is a good reason why we do this which we could put in a comment. http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode401 src/gdbjit.cc:401: #endif At least one place in this file should have an else and a #error to catch the ARM case and give a sensible error message. http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.cc#newcode858 src/gdbjit.cc:858: is_statement = !is_statement; How about is_statement = info->is_statement_; http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h File src/gdbjit.h (right): http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode31 src/gdbjit.h:31: #ifdef ENABLE_GDBJIT_INTERFACE A few lines here on which gdb versions, OSs etc. this is expected to work on would be nice. http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode59 src/gdbjit.h:59: void SetPosition(intptr_t pc, int pos, bool is_statement) { What is pos, a line number? The names of the function and the variables should tell us more. http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode72 src/gdbjit.h:72: List<PCInfo>* pc_info() { Lower case names are reserved for getters. Doing a sort means it is not just a getter. Either the function should be renamed or the sort should be moved out. http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode82 src/gdbjit.h:82: return a->pc_ - b->pc_; Subtracting two 64 bit values and returning the answer as a signed 32 bit value is rather fragile even though it probably works right now. http://codereview.chromium.org/5965011/diff/12016/src/gdbjit.h#newcode91 src/gdbjit.h:91: There should be two blank lines here. http://codereview.chromium.org/5965011/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
