First batch of comments.
http://codereview.chromium.org/3606009/diff/1/2 File src/arm/assembler-arm.cc (right): http://codereview.chromium.org/3606009/diff/1/2#newcode1641 src/arm/assembler-arm.cc:1641: ASSERT(code >= -1); // code should be positive, except for the default -1. Could you move the comment before the ASSERT and capitalize the first word? http://codereview.chromium.org/3606009/diff/1/2#newcode1647 src/arm/assembler-arm.cc:1647: static uint32_t index = 0; Should we make stops without a code unwatched? That would avoid accidental overlap between stop with a code and stop without a code. I don't think it is unreasonable to require a specified code if you want the stop to be watched? http://codereview.chromium.org/3606009/diff/1/3 File src/arm/assembler-arm.h (right): http://codereview.chromium.org/3606009/diff/1/3#newcode907 src/arm/assembler-arm.h:907: // See assembler-arm.cc for details. I don't think this addition to the comment adds much. Remove? http://codereview.chromium.org/3606009/diff/1/7 File src/arm/simulator-arm.cc (right): http://codereview.chromium.org/3606009/diff/1/7#newcode120 src/arm/simulator-arm.cc:120: char* msg = * msg_address; No space between '*' and 'msg_address' or dereference directly on line above as in the code below. http://codereview.chromium.org/3606009/diff/1/7#newcode124 src/arm/simulator-arm.cc:124: if (isWatchedStop(code) && !watched_stops[code].desc) Please add braces around the body. http://codereview.chromium.org/3606009/diff/1/7#newcode151 src/arm/simulator-arm.cc:151: if (sim_->isWatchedStop(code) && !sim_->watched_stops[code].desc) Please add braces around the body. http://codereview.chromium.org/3606009/diff/1/7#newcode474 src/arm/simulator-arm.cc:474: sim_->EnableStop(value); Indentation is off. http://codereview.chromium.org/3606009/diff/1/7#newcode485 src/arm/simulator-arm.cc:485: sim_->DisableStop(value); Indentation is off. http://codereview.chromium.org/3606009/diff/1/7#newcode531 src/arm/simulator-arm.cc:531: Simulator::kNumOfWatchedStops); Identation is off. http://codereview.chromium.org/3606009/diff/1/7#newcode1594 src/arm/simulator-arm.cc:1594: if (isWatchedStop(code)) Please add braces. http://codereview.chromium.org/3606009/diff/1/7#newcode1636 src/arm/simulator-arm.cc:1636: if (!isEnabledStop(code)) Please add braces. http://codereview.chromium.org/3606009/diff/1/7#newcode1643 src/arm/simulator-arm.cc:1643: if (isEnabledStop(code)) braces. http://codereview.chromium.org/3606009/diff/1/7#newcode1674 src/arm/simulator-arm.cc:1674: code, code, state, count, watched_stops[code].desc); Indentation. http://codereview.chromium.org/3606009/diff/1/7#newcode1677 src/arm/simulator-arm.cc:1677: code, code, state, count); Indentation. http://codereview.chromium.org/3606009/diff/1/8 File src/arm/simulator-arm.h (right): http://codereview.chromium.org/3606009/diff/1/8#newcode243 src/arm/simulator-arm.h:243: inline bool isWatchedStop(uint32_t bkpt_code); I don't understand the watched stop distinction. When would you want to have a stop that is not watched? http://codereview.chromium.org/3606009/diff/1/8#newcode345 src/arm/simulator-arm.h:345: // Breakpoint is disabled if bit 31 is set. Please add newline before this comment. http://codereview.chromium.org/3606009/diff/1/8#newcode347 src/arm/simulator-arm.h:347: // A stop is enabled, meaning the simulator will stop when meeting the Please add newline before this comment. http://codereview.chromium.org/3606009/diff/1/8#newcode348 src/arm/simulator-arm.h:348: // instruction, if watched_stops[code].count is positive. This comment does not make sense since count is unsigned. If bit 31 of ... is not set. http://codereview.chromium.org/3606009/diff/1/8#newcode351 src/arm/simulator-arm.h:351: struct stop_count_and_desc { Structs must have CamelCase names. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_Names#Type_Names http://codereview.chromium.org/3606009/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
