ARM parts LGTM.
Zaheer mentioned that he had a test that crashed with an earlier version of
the
patch. It would be very good if that test could be made into a form where
it
could be committed along with this change.
http://codereview.chromium.org/6170001/diff/90001/src/arm/macro-assembler-arm.h
File src/arm/macro-assembler-arm.h (right):
http://codereview.chromium.org/6170001/diff/90001/src/arm/macro-assembler-arm.h#newcode259
src/arm/macro-assembler-arm.h:259: // |stack_space| pending pushes, used
for alignment before call to C.
On 2011/01/21 17:56:36, antonm wrote:
Erik, it was my request. I saw this style in various parts of v8 and
assumed
it's the default. Please, tell us your ultimate word on it.
Sorry for causing this confusion. I didn't check properly, and the
style is used elsewhere in the file. We are not very consistent in the
format of these comments. I think both what Zaheer originally wrote and
what he has now are both fine.
Zaheer, sorry for luring you into this.
On 2011/01/21 14:28:40, Erik Corry wrote:
> We haven't used this |parameter_name| style elsewhere as far as I
can see.
> Also, this comment has not been updated to reflect that the ip has
sp that
> should be restored. Also the comment is out of date because it
mentions r6 as
> being set up by the EnterExitFrame, whereas in fact it needs to be
set up by
the
> caller.
http://codereview.chromium.org/6170001/diff/55003/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):
http://codereview.chromium.org/6170001/diff/55003/src/arm/macro-assembler-arm.cc#newcode1398
src/arm/macro-assembler-arm.cc:1398:
You should have 2 blank lines between functions.
http://codereview.chromium.org/6170001/diff/55003/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):
http://codereview.chromium.org/6170001/diff/55003/src/arm/simulator-arm.cc#newcode1600
src/arm/simulator-arm.cc:1600: } else {
The usual pattern in V8 is to add a line here saying something like.
ASSERT(redirection->type() == ExternalReference::...)
which serves as a check and also a comment that aids the reader.
http://codereview.chromium.org/6170001/diff/55003/src/arm/simulator-arm.h
File src/arm/simulator-arm.h (right):
http://codereview.chromium.org/6170001/diff/55003/src/arm/simulator-arm.h#newcode289
src/arm/simulator-arm.h:289: v8::internal::ExternalReference::Type
type);
The indentation of these two parameters is wrong.
http://codereview.chromium.org/6170001/diff/55003/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):
http://codereview.chromium.org/6170001/diff/55003/src/arm/stub-cache-arm.cc#newcode595
src/arm/stub-cache-arm.cc:595: static bool
GenerateFastApiDirectCall(MacroAssembler* masm,
Instead of returning a bool and using a Failure** to return the failure
this function should return a MaybeObject*. This is the pattern used
throughout V8 for functions that may encounter an allocation failure.
You can return Heap::undefined_value() in the success cases where there
is nothing to return.
http://codereview.chromium.org/6170001/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
