LGTM with a few comments:
http://codereview.chromium.org/3226014/diff/1/4 File src/frames.cc (right): http://codereview.chromium.org/3226014/diff/1/4#newcode158 src/frames.cc:158: if (type == StackFrame::JAVA_SCRIPT) { Remove this code or guard it by #ifdef DEBUG and assert that we can always find the code for JavaScript frames. As it is written now, it doesn't make much sense. http://codereview.chromium.org/3226014/diff/1/4#newcode824 src/frames.cc:824: Address pc) { Weird indentation. Use only 4 spaces. http://codereview.chromium.org/3226014/diff/1/4#newcode831 src/frames.cc:831: index++; I'm not sure I understand why you increment index here. Index isn't used from this point forward. http://codereview.chromium.org/3226014/diff/1/5 File src/frames.h (right): http://codereview.chromium.org/3226014/diff/1/5#newcode51 src/frames.h:51: static const int kPcToCodeCacheSize = 256; Does the size have to be public? http://codereview.chromium.org/3226014/diff/1/5#newcode64 src/frames.h:64: // static void FlushPcToCodeCache(); Remove comment or change it to something that makes sense. http://codereview.chromium.org/3226014/diff/1/5#newcode70 src/frames.h:70: Remove one of the empty lines here. http://codereview.chromium.org/3226014/diff/1/5#newcode73 src/frames.h:73: Remove two empty lines here. http://codereview.chromium.org/3226014/diff/1/5#newcode191 src/frames.h:191: Remove one of the two newlines. http://codereview.chromium.org/3226014/diff/1/5#newcode195 src/frames.h:195: return (PcToCodeCache::GetCacheEntry(pc))->code; Remove one level of ()s. http://codereview.chromium.org/3226014/diff/1/13 File src/spaces.cc (right): http://codereview.chromium.org/3226014/diff/1/13#newcode2732 src/spaces.cc:2732: // TODO(kasperl): Change this implementation to only find executable File an issue and change the TODO to include the issue number. http://codereview.chromium.org/3226014/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
