Implement try/catch without finally, and without type checks.

Implement try/catch without finally, and without type checks.
Committed: https://code.google.com/p/dart/source/detail?r=4026

[floitsch, 2012-02-07 17:40:00]

Didn't have the time to finish the review.
Just some comments for now.

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/builder...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:1727: if (!isAborted()) endTryBlock = close(new
HGoto());
We usually just update the variable:
tryBlock = close(new HGoto());

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:1759: HBasicBlock joinBlock = graph.addNewBlock();
now that I think about it, I'm not sure we need the joinBlock.

[ahe, 2012-02-07 19:10:35]

DBC diagnostics suggestions.

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/resolver.da...
frog/leg/resolver.dart:849: error(node, MessageKind.NO_CATCH_NOR_FINALLY);
The precise location here is:

node.getEndToken.next

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/resolver.da...
frog/leg/resolver.dart:861: error(node.formals.nodes.tail.tail.head,
} else if (!node.formals.nodes.tail.isEmpty()) {
  for (Node extra in node.formals.nodes.tail.tail) {
   error(extra, ...)
  }

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/warnings.dart
File frog/leg/warnings.dart (right):

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/warnings.da...
frog/leg/warnings.dart:90: 'expected a catch block or finally block after a try
block');
I suggest shortening this error to:

expected 'catch' or 'finally'.

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/warnings.da...
frog/leg/warnings.dart:94: 'too many variables in a catch declaration');
I'd stick to the singular case and report one error per extra variable:

'extra variable in catch declaration'

[ngeoffray, 2012-02-08 07:56:02]

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/bailout...
File frog/leg/ssa/bailout.dart (right):

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/bailout...
frog/leg/ssa/bailout.dart:199: visitBasicBlock(successors[i]);
I know this is wrong. I plan on adding test cases for bailouts to make sure we
visit the right order and the liveness of variables is computed correctly.

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/bailout...
frog/leg/ssa/bailout.dart:340: }
ditto.

[ngeoffray, 2012-02-08 09:52:23]

Thanks for the early feedback.

http://codereview.chromium.org/9351020/diff/3001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9351020/diff/3001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:849: error(node, MessageKind.NO_CATCH_NOR_FINALLY);
On 2012/02/07 19:10:35, ahe wrote:
> The precise location here is:
> 
> node.getEndToken.next

Added a TODO because there is no API for compiler where reportError takes a
token.

http://codereview.chromium.org/9351020/diff/3001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:861: error(node.formals.nodes.tail.tail.head,
On 2012/02/07 19:10:35, ahe wrote:
> } else if (!node.formals.nodes.tail.isEmpty()) {
>   for (Node extra in node.formals.nodes.tail.tail) {
>    error(extra, ...)
>   }

Done.

http://codereview.chromium.org/9351020/diff/3001/frog/leg/ssa/bailout.dart
File frog/leg/ssa/bailout.dart (right):

http://codereview.chromium.org/9351020/diff/3001/frog/leg/ssa/bailout.dart#ne...
frog/leg/ssa/bailout.dart:199: visitBasicBlock(successors[i]);
On 2012/02/08 07:56:03, ngeoffray wrote:
> I know this is wrong. I plan on adding test cases for bailouts to make sure we
> visit the right order and the liveness of variables is computed correctly.

After discussions, decided to not do any speculative optimizations on try/catch
for now.

http://codereview.chromium.org/9351020/diff/3001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9351020/diff/3001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:1727: if (!isAborted()) endTryBlock = close(new
HGoto());
On 2012/02/07 17:40:00, floitsch wrote:
> We usually just update the variable:
> tryBlock = close(new HGoto());

As discussed, renamed the variable to endTryBody.

http://codereview.chromium.org/9351020/diff/3001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:1759: HBasicBlock joinBlock = graph.addNewBlock();
On 2012/02/07 17:40:00, floitsch wrote:
> now that I think about it, I'm not sure we need the joinBlock.

Done.

http://codereview.chromium.org/9351020/diff/3001/frog/leg/warnings.dart
File frog/leg/warnings.dart (right):

http://codereview.chromium.org/9351020/diff/3001/frog/leg/warnings.dart#newco...
frog/leg/warnings.dart:90: 'expected a catch block or finally block after a try
block');
On 2012/02/07 19:10:35, ahe wrote:
> I suggest shortening this error to:
> 
> expected 'catch' or 'finally'.

Done.

http://codereview.chromium.org/9351020/diff/3001/frog/leg/warnings.dart#newco...
frog/leg/warnings.dart:94: 'too many variables in a catch declaration');
On 2012/02/07 19:10:35, ahe wrote:
> I'd stick to the singular case and report one error per extra variable:
> 
> 'extra variable in catch declaration'

Done.

[floitsch, 2012-02-08 10:07:52]

LGTM.

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/closure...
File frog/leg/ssa/closure.dart (right):

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/closure...
frog/leg/ssa/closure.dart:68: bool inTryCatchOrFinally = false;
we don't care about the try-body. -> inCatchOrFinally.

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/closure...
frog/leg/ssa/closure.dart:155: // TODO(ngeoffray): only do this if the variable
is mutated.
only do this if the variable is mutated in the try-body.

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/closure...
frog/leg/ssa/closure.dart:318: visitTryStatement(TryStatement node) {
move this code into visitCatch and visitFinally.

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/tracer....
File frog/leg/ssa/tracer.dart (right):

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/tracer....
frog/leg/ssa/tracer.dart:385: String visitTry(HTry node) {
Did you test this in the c1visualizer?

[ngeoffray, 2012-02-08 10:17:17]

Thanks Florian.

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/closure...
File frog/leg/ssa/closure.dart (right):

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/closure...
frog/leg/ssa/closure.dart:68: bool inTryCatchOrFinally = false;
On 2012/02/08 10:07:53, floitsch wrote:
> we don't care about the try-body. -> inCatchOrFinally.

As discussed, we do care about the try body, because any variable mutation into
it affects code after the try/catch/finally.

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/closure...
frog/leg/ssa/closure.dart:155: // TODO(ngeoffray): only do this if the variable
is mutated.
On 2012/02/08 10:07:53, floitsch wrote:
> only do this if the variable is mutated in the try-body.

ditto

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/closure...
frog/leg/ssa/closure.dart:318: visitTryStatement(TryStatement node) {
On 2012/02/08 10:07:53, floitsch wrote:
> move this code into visitCatch and visitFinally.

ditto

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/tracer....
File frog/leg/ssa/tracer.dart (right):

https://chromiumcodereview.appspot.com/9351020/diff/3001/frog/leg/ssa/tracer....
frog/leg/ssa/tracer.dart:385: String visitTry(HTry node) {
On 2012/02/08 10:07:53, floitsch wrote:
> Did you test this in the c1visualizer?

Just did, and it works :)

[ahe, 2012-02-08 10:19:27]

http://codereview.chromium.org/9351020/diff/3001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9351020/diff/3001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:849: error(node, MessageKind.NO_CATCH_NOR_FINALLY);
On 2012/02/08 09:52:23, ngeoffray wrote:
> On 2012/02/07 19:10:35, ahe wrote:
> > The precise location here is:
> > 
> > node.getEndToken.next
> 
> Added a TODO because there is no API for compiler where reportError takes a
> token.

Please file a bug and assign it to me.

Cheers,
Peter

[ngeoffray, 2012-02-08 10:23:21]

http://codereview.chromium.org/9351020/diff/3001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9351020/diff/3001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:849: error(node, MessageKind.NO_CATCH_NOR_FINALLY);
On 2012/02/08 10:19:27, ahe wrote:
> On 2012/02/08 09:52:23, ngeoffray wrote:
> > On 2012/02/07 19:10:35, ahe wrote:
> > > The precise location here is:
> > > 
> > > node.getEndToken.next
> > 
> > Added a TODO because there is no API for compiler where reportError takes a
> > token.
> 
> Please file a bug and assign it to me.
> 
> Cheers,
> Peter

Done: http://code.google.com/p/dart/issues/detail?id=1581

[ahe, 2012-02-08 10:27:12]

http://codereview.chromium.org/9351020/diff/3001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9351020/diff/3001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:849: error(node, MessageKind.NO_CATCH_NOR_FINALLY);
On 2012/02/08 10:23:21, ngeoffray wrote:
> On 2012/02/08 10:19:27, ahe wrote:
> > On 2012/02/08 09:52:23, ngeoffray wrote:
> > > On 2012/02/07 19:10:35, ahe wrote:
> > > > The precise location here is:
> > > > 
> > > > node.getEndToken.next
> > > 
> > > Added a TODO because there is no API for compiler where reportError takes
a
> > > token.
> > 
> > Please file a bug and assign it to me.
> > 
> > Cheers,
> > Peter
> 
> Done: http://code.google.com/p/dart/issues/detail?id=1581

Thank you. I can see that you put the issue number in the TODO and this was
exactly what I hoped you'd do!