Nice stuff! A few nits to look at and then lgtm.
https://codereview.chromium.org/23726041/diff/4001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/23726041/diff/4001/src/code-stubs-hydrogen.cc#newcode343
src/code-stubs-hydrogen.cc:343: info()->MarkAsSavesCallerDoubles();
Comment this as to why you are doing it (the runtime call in
BuildNumberToString).
Also, I don't like that we use constant 0,1,2 for parameter numbers. In
ArrayConstructorStubBase, there is code like:
// Parameters accessed via CodeStubGraphBuilder::GetParameter()
static const int kConstructor = 0;
static const int kPropertyCell = 1;
And then GetParameter calls in those stubs use the constants. Could you
similarly define a constant for NumberToStrings parameter?
https://codereview.chromium.org/23726041/diff/4001/src/code-stubs.h
File src/code-stubs.h (right):
https://codereview.chromium.org/23726041/diff/4001/src/code-stubs.h#newcode476
src/code-stubs.h:476:
Here is a good place for something like:
static const int kNumberParameter = 0;
https://codereview.chromium.org/23726041/diff/4001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/23726041/diff/4001/src/hydrogen.cc#newcode1316
src/hydrogen.cc:1316: if_objectissmi.Else();
All this is really nice that you can use the IfBuilder and
JoinContinuation!
https://codereview.chromium.org/23726041/diff/4001/src/hydrogen.cc#newcode9030
src/hydrogen.cc:9030: return
ast_context()->ReturnValue(BuildNumberToString(Pop()));
Paranoia: This code reminds me of the problem that in optimized C++
ast_context() may be evaluated before BuildNumberToString(Pop()). I
would separate that into two lines.
https://codereview.chromium.org/23726041/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
