LGTM, but I do think we need a regression test case. What's wrong with
the code in issue 220 -- can't that be used a a regression test case?
You could even extend it a bit to also cover the const case.
On Sun, Feb 1, 2009 at 9:53 AM, wrote:
> Reviewers: Kasper Lund, Mads Ager, iposva,
>
> Messa
That code hit the debug assert that we had before, but I've removed the
assert with this change (because it isn't true). A test based on the code
in issue 220 passed in release builds before this change and would have
passed in debug builds without the assert.
We should still be able to construct
On Mon, Feb 2, 2009 at 8:04 AM, Kevin Millikin wrote:
> That code hit the debug assert that we had before, but I've removed the
> assert with this change (because it isn't true). A test based on the code
> in issue 220 passed in release builds before this change and would have
> passed in debug
http://codereview.chromium.org/19745/diff/1/2
File src/codegen-arm.cc (right):
http://codereview.chromium.org/19745/diff/1/2#newcode1154
Line 1154: frame_->Pop();
What is it that we are popping here? If it is the value that has been
loaded with "Load(val)" then aren't you doing things in the wro
LGTM as well, especially once a clarifying comment is put in.
-Ivan
http://codereview.chromium.org/19745
--~--~-~--~~~---~--~~
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
-~--~~~~--~~--~--~---
If you add a comment about what we're popping (as requested by Ivan),
it still LGTM.
On Mon, Feb 2, 2009 at 11:12 AM, wrote:
> Added regression test. Take a quick look, please.
>
> http://codereview.chromium.org/19745
>
--~--~-~--~~~---~--~~
v8-dev mailing list
Added regression test. Take a quick look, please.
http://codereview.chromium.org/19745
--~--~-~--~~~---~--~~
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
-~--~~~~--~~--~--~---
