https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-builder.cc
File src/compiler/ast-graph-builder.cc (right):
https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-builder.cc#newcode351
src/compiler/ast-graph-builder.cc:351: return
owner_->NewNode(owner_->javascript()->StrictEqual(), t1, t2);
On 2015/02/02 09:47:29, titzer wrote:
I think you want pointer/integer equality here. JS operators are for
expressing
source program computations, not internal computations.
Done.
https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-builder.h
File src/compiler/ast-graph-builder.h (right):
https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-builder.h#newcode42
src/compiler/ast-graph-builder.h:42: class ContextScope;
On 2015/02/02 09:47:29, titzer wrote:
Do this class and its subclasses really need to be nested in the graph
builder?
Acknowledged. As discussed offline, it gives access to protected members
of AstGraphBuilder. Otherwise all the ControlScope classes would either
need to be friends or we would need to make a ton of methods on
AstGraphBuilder public. Let's refactor this in one go for all nested
classes later.
https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-builder.h#newcode411
src/compiler/ast-graph-builder.h:411: ControlScope(AstGraphBuilder*
owner, int drop_extra)
On 2015/02/02 09:47:29, titzer wrote:
{owner} is a bad name. Would prefer the more verbose
{ast_graph_builder} (and
hopefully in the future simply {graph_builder}) to make this more
clear.
{drop_extra} should be named {expr_stack_delta} or something similar
to make it
more clear.
Done. As discussed offline, used {builder} instead of {owner} and
{stack_delta} instead of {drop_extra}
https://codereview.chromium.org/873423004/diff/1/src/compiler/ast-graph-builder.h#newcode439
src/compiler/ast-graph-builder.h:439: void PerformCommand(Command cmd,
Statement* stmt, Node* value);
On 2015/02/02 09:47:29, titzer wrote:
{stmt} is called {target} in all the subclasses.
Done. Here and in the occurrences below.
https://codereview.chromium.org/873423004/diff/1/src/compiler/graph-builder.h
File src/compiler/graph-builder.h (right):
https://codereview.chromium.org/873423004/diff/1/src/compiler/graph-builder.h#newcode238
src/compiler/graph-builder.h:238: // Friends using the values vector.
On 2015/02/02 09:47:29, titzer wrote:
What if instead of friending, you added public methods Push() and
Pop()?
This will get easier when we put AstGraphBuilder and GraphBuilder back
together
again.
Acknowledged. Yeah, well, we could do that if StructuredGraphBuilder
weren't abstractified out. I placed a TODO here.
https://codereview.chromium.org/873423004/diff/1/test/cctest/compiler/test-run-jsexceptions.cc
File test/cctest/compiler/test-run-jsexceptions.cc (right):
https://codereview.chromium.org/873423004/diff/1/test/cctest/compiler/test-run-jsexceptions.cc#newcode48
test/cctest/compiler/test-run-jsexceptions.cc:48: TEST(Catch) {
On 2015/02/02 09:47:29, titzer wrote:
+ 1
Good to have cctests for TurboFan, yay. Can we also have these tests
as mjsunit?
Redundancy is better than holes in coverage :-)
Also, I foresee some quite nasty interactions with OSR in the future,
so we'll
probably want to have lots of mjsunit tests that can be run in various
modes.
The cases covered by these cases are already covered by try.js in the
mjsunit test suite.
https://codereview.chromium.org/873423004/
--
--
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/d/optout.
