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);
I think you want pointer/integer equality here. JS operators are for
expressing source program computations, not internal computations.
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;
Do this class and its subclasses really need to be nested in the graph
builder?
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)
{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.
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);
{stmt} is called {target} in all the subclasses.
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.
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.
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) {
+ 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.
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.
