Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2008363002:
Add label support to ExitDetector (Closed)
|
Created:
4 years, 7 months ago by srawlins Modified:
4 years, 7 months ago CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd label support to ExitDetector
BUG=https://github.com/dart-lang/sdk/issues/26534
R=brianwilkerson@google.com
Committed: https://github.com/dart-lang/sdk/commit/8fdeb785fdaba5ea2983a70246bb087a14e1125b
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressing comments #
Total comments: 2
Patch Set 3 : Re-short circuit, plus test #
Messages
Total messages: 12 (2 generated)
srawlins@google.com changed reviewers: + brianwilkerson@google.com, scheglov@google.com
https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/resolver.dart:3476: Set<AstNode> _enclosingBlockBreaksLabel = new Set<AstNode>(); This probably isn't a popular decision, whole AstNodes??? But I couldn't find any better way to uniquely identify labels. Does the Identifier of the label work? The String name? Do labels need to be unique across a compilation unit or just a function? https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/resolver.dart:3698: bool thenExits = _nodeExits(thenStatement); Need to always visit `then` and `else`, to find possible break labels. https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/resolver.dart:3934: exits = true; This may also be unpopular. We can't short circuit evaluating ALL of the statements, because we want to catch and break labels inside. Should this require a new visitor? Something complicated like "Visit the statements until you find one that exits. Then visit the others with a BreakLabelVisitor to catch break labels, ...
This doesn't handle the "empty" label. Consider:
while (true) {
if (1 < 2) {
break;
}
return;
}
// code here is not dead
https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/resolver.dart (right):
https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:3476: Set<AstNode>
_enclosingBlockBreaksLabel = new Set<AstNode>();
> This probably isn't a popular decision, whole AstNodes???
As long as the ExitDetector is not long-lived, then it should be fine to store
the nodes.
> But I couldn't find any better way to uniquely identify labels. Does the
Identifier of the label work? The String name?
Nope. You can have multiple labels with the same identifier (unless by
"identifier" you mean the SimpleIdentifier AST node, in which case that's also
unique but also an AST node).
> Do labels need to be unique across a compilation unit or just a function?
Neither. The following is perfectly valid:
main() {
label: while (true) {
label: while (true) {
break label;
}
break label;
}
}
and is equivalent to:
main() {
label1: while (true) {
label2: while (true) {
break label2;
}
break label1;
}
}
https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:3736:
_enclosingBlockBreaksLabel.remove(node);
"node" --> "node.statement"?
https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:3934: exits = true;
> We can't short circuit evaluating ALL of the statements, because we want to
catch and break labels inside.
I don't understand why. If a statement always exits, then the code after it is
dead and any break inside it won't be hit:
label: while (true) {
return;
break label;
}
On 2016/05/25 18:13:24, Brian Wilkerson wrote:
> This doesn't handle the "empty" label. Consider:
>
> while (true) {
> if (1 < 2) {
> break;
> }
> return;
> }
> // code here is not dead
This is already caught by the ExitDetector. It detects that the infinite while
loop contains a break, and determines it to not exit.
>
>
https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera...
> File pkg/analyzer/lib/src/generated/resolver.dart (right):
>
>
https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/resolver.dart:3476: Set<AstNode>
> _enclosingBlockBreaksLabel = new Set<AstNode>();
> > This probably isn't a popular decision, whole AstNodes???
>
> As long as the ExitDetector is not long-lived, then it should be fine to store
> the nodes.
>
> > But I couldn't find any better way to uniquely identify labels. Does the
> Identifier of the label work? The String name?
>
> Nope. You can have multiple labels with the same identifier (unless by
> "identifier" you mean the SimpleIdentifier AST node, in which case that's also
> unique but also an AST node).
>
> > Do labels need to be unique across a compilation unit or just a function?
>
> Neither. The following is perfectly valid:
>
> main() {
> label: while (true) {
> label: while (true) {
> break label;
> }
> break label;
> }
> }
>
> and is equivalent to:
>
> main() {
> label1: while (true) {
> label2: while (true) {
> break label2;
> }
> break label1;
> }
> }
>
>
https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/resolver.dart:3736:
> _enclosingBlockBreaksLabel.remove(node);
> "node" --> "node.statement"?
>
>
https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/resolver.dart:3934: exits = true;
> > We can't short circuit evaluating ALL of the statements, because we want to
> catch and break labels inside.
>
> I don't understand why. If a statement always exits, then the code after it is
> dead and any break inside it won't be hit:
>
> label: while (true) {
> return;
> break label;
> }
Certainly. But if a statement exits, the statements _before_ it might contain
break labels. We must visit them lest we false positively declare the block of
statements to always exit.
In this example:
label: while (true) {
if (1 < 2)
break label;
return;
}
statement;
as the code currently exists, it walks the Block's statements backwards, finds
that `return;` always exits, and returns early, declaring the while loop to
always exit, and `statement` to be dead code. It never visited the if statement,
containing the break label.
> > This doesn't handle the "empty" label. > > This is already caught by the ExitDetector. It detects that the infinite while > loop contains a break, and determines it to not exit. Ok; good. > https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera... > > pkg/analyzer/lib/src/generated/resolver.dart:3934: exits = true; > > > We can't short circuit evaluating ALL of the statements, because we want to > > > catch and break labels inside. > > > > I don't understand why. > > ... as the code currently exists, it walks the Block's statements backwards, finds > that `return;` always exits, and returns early, declaring the while loop to > always exit, and `statement` to be dead code. It never visited the if statement, > containing the break label. Ah. I missed the fact that it's iterating backward. It seems like what we really want to do is iterate forwards so that we aren't fooled by break statements in dead code. (The backward iteration was only there as an optimization, and we've removed the optimization so that it's now pointless to not iterate forwards.)
https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/resolver.dart:3476: Set<AstNode> _enclosingBlockBreaksLabel = new Set<AstNode>(); On 2016/05/25 18:13:23, Brian Wilkerson wrote: > > This probably isn't a popular decision, whole AstNodes??? > > As long as the ExitDetector is not long-lived, then it should be fine to store > the nodes. > > > But I couldn't find any better way to uniquely identify labels. Does the > Identifier of the label work? The String name? > > Nope. You can have multiple labels with the same identifier (unless by > "identifier" you mean the SimpleIdentifier AST node, in which case that's also > unique but also an AST node). > > > Do labels need to be unique across a compilation unit or just a function? > > Neither. The following is perfectly valid: > > main() { > label: while (true) { > label: while (true) { > break label; > } > break label; > } > } > > and is equivalent to: > > main() { > label1: while (true) { > label2: while (true) { > break label2; > } > break label1; > } > } Acknowledged. https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/resolver.dart:3736: _enclosingBlockBreaksLabel.remove(node); On 2016/05/25 18:13:23, Brian Wilkerson wrote: > "node" --> "node.statement"? Done. https://codereview.chromium.org/2008363002/diff/1/pkg/analyzer/lib/src/genera... pkg/analyzer/lib/src/generated/resolver.dart:3934: exits = true; On 2016/05/25 18:13:23, Brian Wilkerson wrote: > > We can't short circuit evaluating ALL of the statements, because we want to > catch and break labels inside. > > I don't understand why. If a statement always exits, then the code after it is > dead and any break inside it won't be hit: > > label: while (true) { > return; > break label; > } OK it walks forwards now.
https://codereview.chromium.org/2008363002/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/2008363002/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:3934: exits = true; So can this return after finding the first statement that exits?
https://codereview.chromium.org/2008363002/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/resolver.dart (right): https://codereview.chromium.org/2008363002/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/resolver.dart:3934: exits = true; On 2016/05/25 20:50:25, Brian Wilkerson wrote: > So can this return after finding the first statement that exits? Brilliant! It can. And I've added another test, to show that its smart about this.
lgtm
Description was changed from ========== Add label support to ExitDetector BUG=https://github.com/dart-lang/sdk/issues/26534 ========== to ========== Add label support to ExitDetector BUG=https://github.com/dart-lang/sdk/issues/26534 R=brianwilkerson@google.com Committed: https://github.com/dart-lang/sdk/commit/8fdeb785fdaba5ea2983a70246bb087a14e1125b ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 8fdeb785fdaba5ea2983a70246bb087a14e1125b (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
