On 2015/02/02 14:47:41, Michael Starzinger wrote:
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.
Ok, can you leave a TODO somewhere, since I think we will want to increase
coverage with some more mjsunit tests for try.
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.
