http://codereview.chromium.org/18089/diff/1/5 File src/jump-target-ia32.cc (right):
http://codereview.chromium.org/18089/diff/1/5#newcode106 Line 106: ASSERT(!cgen_->has_cc()); On 2009/01/15 12:21:54, William Hesse wrote: > This looks like a duplicate line. Oops. Fixed. http://codereview.chromium.org/18089/diff/1/7 File src/jump-target.h (right): http://codereview.chromium.org/18089/diff/1/7#newcode38 Line 38: // TODO(): Update this comment. On 2009/01/15 10:15:02, Kasper Lund wrote: > You should probably just do this. Done. http://codereview.chromium.org/18089/diff/1/7#newcode102 Line 102: expected_frame_ = NULL; On 2009/01/15 10:15:02, Kasper Lund wrote: > Do we need to delete the expected frame here before setting it to NULL? If not, > it should probably be mentioned in a comment. We don't want to delete it because it has normally been copied. I think the comment here before the function is pretty clear. http://codereview.chromium.org/18089/diff/1/7#newcode127 Line 127: // become the current frame after the bind. On 2009/01/15 12:21:54, William Hesse wrote: > Are we allowing non-mergable expected frames, which might be changed to be > mergable or merged when bound, so that the frame after the binding site won't be > exactly the expected frame? The frame after the binding site will always be the expected one. I've just changed the comment to be more specific. http://codereview.chromium.org/18089/diff/1/7#newcode160 Line 160: void AddReachingFrame(VirtualFrame* frame) { On 2009/01/15 10:15:02, Kasper Lund wrote: > I don't know if this really should be inline. Maybe it would be better to move > it to the .cc file? Done. http://codereview.chromium.org/18089/diff/1/7#newcode184 Line 184: // jump target shadows other one, which is hidden as the On 2009/01/15 12:21:54, William Hesse wrote: > Not grammatical, confusing. Added "the". http://codereview.chromium.org/18089/diff/1/7#newcode199 Line 199: JumpTarget* other_target() const { return other_target_; } On 2009/01/15 12:21:54, William Hesse wrote: > Could this be called "active" target? This is the JumpTarget passed to the > constructor, which always seems to be the active one, or the one that is "seen". > The ShadowTarget represents the hidden target while it is masked by another, > and the target that was used to hide the original, after the hiding is over, > right? No. After shadowing they are both active. I can't think of any better name for this thing that is accurate for its whole life. It is simply the "other" target, the one that is not represented by the shadow target. http://codereview.chromium.org/18089/diff/1/4 File src/virtual-frame-ia32.cc (left): http://codereview.chromium.org/18089/diff/1/4#oldcode321 Line 321: cgen_->SetFrame(this, &non_frame_registers); On 2009/01/15 12:21:54, William Hesse wrote: > Can you put an assert in, that the frame is its cgen's current frame? Done. http://codereview.chromium.org/18089 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
