Fix issue 5517 by setting the successors the right way in a try/catch.

Fix issue 5517 by setting the successors the right way in a try/catch.
Committed: https://code.google.com/p/dart/source/detail?r=12998

[ngeoffray, 2012-09-27 15:10:05]

https://codereview.chromium.org/10987073/diff/2001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/nodes.dart (left):

https://codereview.chromium.org/10987073/diff/2001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/nodes.dart:585: assert(isClosed() &&
(block.isNew() || block.id < id));
This assert turns out not being very useful, and making addSuccessor not
flexible enough.

[Søren Gjesse, 2012-09-27 15:35:28]

lgtm, but you better get Kaspers comments as well.

https://codereview.chromium.org/10987073/diff/2001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/10987073/diff/2001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3947: } else {
Do you ever get here? You will always have at least catch or finally.

https://codereview.chromium.org/10987073/diff/2001/tests/language/finally_tes...
File tests/language/finally_test.dart (right):

https://codereview.chromium.org/10987073/diff/2001/tests/language/finally_tes...
tests/language/finally_test.dart:3: // BSD-style license that can be found in
the LICENSE file.
This test could also be calle regress_5517_test.dart or regress/5517_test.dart.

https://codereview.chromium.org/10987073/diff/2001/tests/language/finally_tes...
tests/language/finally_test.dart:6: // block would not be seen by the finally
block.
Please mention the issue number if not renaming the test.

[kasperl, 2012-09-28 06:20:06]

LGTM.

https://codereview.chromium.org/10987073/diff/2001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/10987073/diff/2001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3816: HBasicBlock startBodyBlock;
startTryBlock?
endTryBlock?

https://codereview.chromium.org/10987073/diff/2001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3933:
enterBlock.addSuccessor(startCatchBlock);
Maybe add a helper for adding an optional successor (adding it if it isn't
null). Use it in these two cases.

https://codereview.chromium.org/10987073/diff/2001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3943: if (startCatchBlock != null)
{
If you let the addOptionalSuccessor helper return a bool that indicates whether
or not you added the successor you should be able to use it here. Something
like:

   if (!endBodyBlock.addOptionalSuccessor(startCatchBlock) &&
       !endBodyBlock.addOptionalSuccessor(startFinallBlock)) {
     endBodyBlock.addSuccessor(exitBlock);
   }

Not sure it's that much better but at least it's an option.

[ngeoffray, 2012-09-28 08:20:51]

Thanks Kasper and Soren!

https://codereview.chromium.org/10987073/diff/2001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/10987073/diff/2001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3816: HBasicBlock startBodyBlock;
On 2012/09/28 06:20:06, kasperl wrote:
> startTryBlock?
> endTryBlock?

Done.

https://codereview.chromium.org/10987073/diff/2001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3933:
enterBlock.addSuccessor(startCatchBlock);
On 2012/09/28 06:20:06, kasperl wrote:
> Maybe add a helper for adding an optional successor (adding it if it isn't
> null). Use it in these two cases.

Done.

https://codereview.chromium.org/10987073/diff/2001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3943: if (startCatchBlock != null)
{
On 2012/09/28 06:20:06, kasperl wrote:
> If you let the addOptionalSuccessor helper return a bool that indicates
whether
> or not you added the successor you should be able to use it here. Something
> like:
> 
>    if (!endBodyBlock.addOptionalSuccessor(startCatchBlock) &&
>        !endBodyBlock.addOptionalSuccessor(startFinallBlock)) {
>      endBodyBlock.addSuccessor(exitBlock);
>    }
> 
> Not sure it's that much better but at least it's an option.

Soren made a good point that I actually did not need the last if. I changed the
code to use a conditional expression instead.

https://codereview.chromium.org/10987073/diff/2001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:3947: } else {
On 2012/09/27 15:35:28, Søren Gjesse wrote:
> Do you ever get here? You will always have at least catch or finally.

Good catch. Done.

https://codereview.chromium.org/10987073/diff/2001/tests/language/finally_tes...
File tests/language/finally_test.dart (right):

https://codereview.chromium.org/10987073/diff/2001/tests/language/finally_tes...
tests/language/finally_test.dart:3: // BSD-style license that can be found in
the LICENSE file.
On 2012/09/27 15:35:28, Søren Gjesse wrote:
> This test could also be calle regress_5517_test.dart or
regress/5517_test.dart.

I don't really like giving 'regress_#' names to test, makes it impossible to
know what it is about by just looking at the name.

https://codereview.chromium.org/10987073/diff/2001/tests/language/finally_tes...
tests/language/finally_test.dart:6: // block would not be seen by the finally
block.
On 2012/09/27 15:35:28, Søren Gjesse wrote:
> Please mention the issue number if not renaming the test.

Done.