Good point about the asserts. I've committed without them for now. I actually plan to get rid of all those sites with the next change. I'll introduce functions to move one location to another, and to move a literal's (or other trivial expression's) value to a location. Once we have those, the asserts will be there rather than at every call site.
On Mon, Oct 26, 2009 at 6:57 PM, <[email protected]> wrote: > LGTM. > > Small comments about inserting ASSERTs to be more consistent in our code. > > > http://codereview.chromium.org/339004/diff/1/7 > File src/arm/fast-codegen-arm.cc (right): > > http://codereview.chromium.org/339004/diff/1/7#newcode480 > Line 480: } > Add an assert here to be consistent with the rest of the code: > > else { ASSERT(destination.is_nowhere()); } > > http://codereview.chromium.org/339004/diff/1/7#newcode574 > Line 574: if (destination.is_temporary()) __ push(r0); > May want to add an assert here to be consistent with the rest of the > code: > > else ASSERT(destination.is_nowhere()); > > http://codereview.chromium.org/339004/diff/1/7#newcode581 > Line 581: } > Add an assert: > > else { > ASSERT(destination.is_nowhere()); > } > > http://codereview.chromium.org/339004/diff/1/2 > File src/ia32/fast-codegen-ia32.cc (right): > > http://codereview.chromium.org/339004/diff/1/2#newcode475 > Line 475: __ push(eax); > Add an assert here to be consistent with the rest of the code: > > else { ASSERT(destination.is_nowhere()); } > > http://codereview.chromium.org/339004/diff/1/2#newcode571 > Line 571: if (destination.is_temporary()) __ push(eax); > May want to add an assert here to be consistent with the rest of the > code: > > else ASSERT(destination.is_nowhere()); > > http://codereview.chromium.org/339004/diff/1/2#newcode582 > Line 582: __ pop(eax); > Adding an assert to be consistent on all platforms: > > ASSERT(destination.is_nowhere()); > > http://codereview.chromium.org/339004/diff/1/4 > File src/x64/fast-codegen-x64.cc (right): > > http://codereview.chromium.org/339004/diff/1/4#newcode487 > Line 487: } > Add an assert here to be consistent with the rest of the code: > > else { ASSERT(destination.is_nowhere()); } > > http://codereview.chromium.org/339004/diff/1/4#newcode582 > Line 582: if (destination.is_temporary()) __ push(rax); > May want to add an assert here to be consistent with the rest of the > code: > > else ASSERT(destination.is_nowhere()); > > http://codereview.chromium.org/339004/diff/1/4#newcode593 > Line 593: __ pop(rax); > Adding an assert to be consistent on all platforms: > > ASSERT(destination.is_nowhere()); > > > http://codereview.chromium.org/339004 > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
