Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(245)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 9538017: Get rid of CHECK_ALIVE, test only is_open during sequence node traversal. (Closed)

Created:
8 years, 9 months ago by srdjan
Modified:
8 years, 9 months ago
CC:
reviews_dartlang.org, vm-dev_dartlang.org
Visibility:
Public.

Description

Get rid of CHECK_ALIVE, test only is_open during sequence node traversal. Committed: https://code.google.com/p/dart/source/detail?r=4804

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 4

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -25 lines) Patch
M runtime/vm/flow_graph_builder.cc View 1 2 3 11 chunks +6 lines, -25 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
srdjan
8 years, 9 months ago (2012-02-29 19:47:00 UTC) #1
Kevin Millikin (Google)
There's one more place we need a check. Do you agree? If so, LGTM with ...
8 years, 9 months ago (2012-03-01 09:13:53 UTC) #2
srdjan
8 years, 9 months ago (2012-03-01 16:54:58 UTC) #3 
https://chromiumcodereview.appspot.com/9538017/diff/4001/runtime/vm/flow_grap...
File runtime/vm/flow_graph_builder.cc (right):

https://chromiumcodereview.appspot.com/9538017/diff/4001/runtime/vm/flow_grap...
runtime/vm/flow_graph_builder.cc:147: Append(for_effect);
On 2012/03/01 09:13:53, kmillikin wrote:
> I think we also need
> 
> if (!is_open()) return;
> 
> immediately after this call to append.  I can't construct an example because
we
> bail out on TryCatchNode.
> 
> I think the rule is: before connecting a successor to the graph fragment of an
> EffectGraphVisitor, check and handle !is_open().

Done.

https://chromiumcodereview.appspot.com/9538017/diff/4001/runtime/vm/flow_grap...
runtime/vm/flow_graph_builder.cc:1041:
parsed_function().node_sequence()->Visit(&for_effect);
On 2012/03/01 09:13:53, kmillikin wrote:
> And after this call, I think we should be able to assert
!for_effect.is_open(). 
> (If that's not true today, we should make it true).

Done.

Powered by Google App Engine
This is Rietveld 408576698