Simplify the desugaring of catch clauses.

Simplify the desugaring of catch clauses.

Rather than if/then/continue (where the continue does not match the
semantics of Dart's continue), use if/then/else.  This removes the
unnecessary goto and label as a step toward streamlining support for
jumping in the compiler backend.

R=hausner@google.com

Committed: https://code.google.com/p/dart/source/detail?r=30227

[Kevin Millikin (Google), 2013-11-07 13:43:23]

It seems like the dust has settled around try/catch for now so here's the change
to remove the goto and label from catch clauses.

Where before we had:

{
  parameters
  if (type-test) {
    catch-body
    goto label
  }
}
{
  parameters
  if (type-test) {
    catch-body
    goto label
  }
}

We now have:

if (type-test) {
  parameters
  {
    catch-body
  }
} else if (type-test) {
  parameters
  {
    catch-body
  }
}

[hausner, 2013-11-07 17:48:34]

I am fine with eliminating the goto and replace the if-statements with if-else
if. But you can't make parsing recursive. We make an effort to avoid recursion
because we don't want the parser to crash with a stack overflow (think malicious
input with 1000 catch clauses). The security guys will immediately file a bug
against this.

Please rewrite this iteratively. And please do not gratuitously change the name
of functions and data structures in the parser, or change includes without
explanation why.

[Kevin Millikin (Google), 2013-11-07 21:07:56]

On 2013/11/07 17:48:34, hausner wrote:
> I am fine with eliminating the goto and replace the if-statements with if-else
> if. But you can't make parsing recursive. We make an effort to avoid recursion
> because we don't want the parser to crash with a stack overflow (think
malicious
> input with 1000 catch clauses). The security guys will immediately file a bug
> against this.

Hmmm, yeah.  To be fair, I didn't make the parser recursive, it is recursive.  I
think it's much more likely that malicious input would just use 'if(e){' nested
than a sequence of 'catch(e){' --- it's fewer characters and takes four
recursive calls per if instead of one per catch (I'm not sure about the size of
the frames).

I can rewrite it to be iterative, but maybe we should loop in the security guys
first to make sure they think it's worth it.  It's better to implement a stack
guard against overflow.  That's on my todo list, but I wanted to get some
smaller changes done first.

> 
> Please rewrite this iteratively. And please do not gratuitously change the
name
> of functions and data structures in the parser, or change includes without
> explanation why.

The basic reason for all the include changes was that the file used something
from a file that wasn't included, or that it didn't use anything from a file
that was included.  It's too late at night here for me to go through them all,
so I'll get back to you with a full accounting tomorrow.

I do not gratuitously change names.  What would be the point?  Which name
changes did you have in mind?

[hausner, 2013-11-07 21:42:06]

The parser is recursive in some cases where the grammar itself is recursive. It
is not recursive where it does not need to be. We don't parse a statement
sequence with recursive calls to ParseStatement, for example.

Your recursive ParseCatchClause is trivially converted to a loop. With a loop,
it's also trivial to determine whether the ThrowNode is necessary or not, and
you don't have to rely on dead code elimination to remove it again if it's not
needed.

I don't know what you are trying to fix by renaming CatchParamDesc to
CatchParameter, for one example of a gratuitous change.

[Kevin Millikin (Google), 2013-11-07 21:58:31]

On 2013/11/07 21:42:06, hausner wrote:
> The parser is recursive in some cases where the grammar itself is recursive.
It
> is not recursive where it does not need to be. We don't parse a statement
> sequence with recursive calls to ParseStatement, for example.
> 
> Your recursive ParseCatchClause is trivially converted to a loop. With a loop,
> it's also trivial to determine whether the ThrowNode is necessary or not, and
> you don't have to rely on dead code elimination to remove it again if it's not
> needed.

Yeah, I do know how to turn recursion into iteration.  I even wrote a
dissertation about it once.

I'm happy to omit the unnecessary throw.  In fact, I'd rather just omit all the
unreachable catch bodies (parsing them of course).  That seems like a clear
improvement over the current state of affairs.

But do you think I should?  I've gotten pushback before over premature
optimization (not necessarily on this project), and the standard for that is
somewhat arbitrary (i.e., if you get a reviewer who just wants to make
friction....)

> 
> I don't know what you are trying to fix by renaming CatchParamDesc to
> CatchParameter, for one example of a gratuitous change.

It's not really a parameter description if it also holds the LocalVariable we've
added so we don't have to look it up later (which is a bit indirect, and
probably error prone if we jigger the scopes around or something).  It just *is*
a wrapper around the parameter.  Besides, code almost always reads better if you
spell things out (within reason, we only get 80 characters), and this name is
the same length as the previous one.

If you like CatchParamDesc much better I can just change it back, but that
actually does seem gratuitous.

[Kevin Millikin (Google), 2013-11-07 22:08:17]

> If you like CatchParamDesc much better I can just change it back, but that
> actually does seem gratuitous.

I should also point out that I've removed this struct from the parser's API by
making the helper function static in the .cc file instead of a member function
of the parser.

So the name itself probably doesn't matter a lot except for readability.  All
other things being equal, I prefer CatchParameter to CatchParamDesc.  What do
you think?

[Kevin Millikin (Google), 2013-11-08 11:03:55]

(We should actually be asked in code reviews to justify leaving includes out,
because that's where all the implicit dependencies creep in :) ).

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.cc
File runtime/vm/parser.cc (left):

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.cc#oldc...
runtime/vm/parser.cc:8: #include "vm/bigint_operations.h"
This file defines a single type which is not used here at all.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:8: #include "platform/utils.h"
This file uses inline functions from class Utils.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:17: #include "vm/handles.h"
Uses the macro HANDLESCOPE, needs its definition.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:18: #include "vm/heap.h"
Uses the enumeration values Heap::kOld and Heap::kNew, needs the definition.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:19: #include "vm/isolate.h"
Uses inline function Isolate::Current().

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:21: #include "vm/native_arguments.h"
Uses inline function NativeArguments::ParameterCountForResolution.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:25: #include "vm/os.h"
Inline functions from class OS.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:27: #include "vm/scanner.h"
Needs definition of class Scanner (uses constants from it).

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:31: #include "vm/timer.h"
Class TimerScope.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:32: #include "vm/zone.h"
Class StackZone.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h
File runtime/vm/parser.h (left):

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#oldco...
runtime/vm/parser.h:14: #include "vm/scanner.h"
scanner.h declares a single type Scanner, which is not used here.  No reason to
include it.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#oldco...
runtime/vm/parser.h:20: class Function;
Not needed if we include object.h.  Otherwise, we can try to forward *all* the
classes we use from object.h, but it seems random to just have one or two.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#oldco...
runtime/vm/parser.h:22: class LiteralToken;
Not needed if we include token.h (which we need to include anyway).

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#oldco...
runtime/vm/parser.h:31: struct CatchParamDesc;
I've moved the only helper that uses this type to a static function in the .cc
file, so this type does not need to be part of the Parser's API.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#oldco...
runtime/vm/parser.h:32: struct FieldInitExpression;
This is just dead.  There is no definition and no uses.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h
File runtime/vm/parser.h (right):

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#newco...
runtime/vm/parser.h:10: #include "platform/assert.h"
This file uses the ASSERT macro and so needs the definition.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#newco...
runtime/vm/parser.h:11: #include "platform/globals.h"
This file uses the PRINTF_ATTRIBUTE macro and so needs the definition.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#newco...
runtime/vm/parser.h:13: #include "vm/allocation.h"
This file uses base classes ZoneAllocated and ValueObject, so it needs their
definitions.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#newco...
runtime/vm/parser.h:14: #include "vm/ast.h"
Note that this file doesn't actually need ast.h, just about a dozen or so
forward definitions.  It seems more convenient just to keep this here even
though it potentially creates implicit dependencies elsewhere.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#newco...
runtime/vm/parser.h:17: #include "vm/object.h"
This file needs a long list of types from object.h.  Some of them were forward
declared already, but by no means all.  (I think it only needs forward
declarations, but I opted for include for the same reason we include ast.h for
consistency.)

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#newco...
runtime/vm/parser.h:18: #include "vm/raw_object.h"
Needs types from raw_object.h.  Probably could do with a bunch of forward
declarations.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#newco...
runtime/vm/parser.h:19: #include "vm/token.h"
This file needs the definition of Token::Kind.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#newco...
runtime/vm/parser.h:26: class LocalScope;
This class and the next two need forward declarations.

https://codereview.chromium.org/63983005/diff/30001/runtime/vm/parser.h#newco...
runtime/vm/parser.h:29: template <typename T> class GrowableArray;
Likewise.

[Kevin Millikin (Google), 2013-11-08 13:11:58]

Please take another look.

[hausner, 2013-11-08 21:52:30]

Nesting of the catch clause nodes is wrong. See comment below.

https://codereview.chromium.org/63983005/diff/140001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/63983005/diff/140001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6653: struct CatchParameter {
As I've aked in previous rounds of this review, please do not rename this.

https://codereview.chromium.org/63983005/diff/140001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6666: static bool AddCatchParamsToScope(CatchParameter*
exception_param,
Please leave this as a method of the parser and keep the error handling where it
is now.

https://codereview.chromium.org/63983005/diff/140001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6910: current = CloseBlock();
I don't think the way you nest the blocks is correct. These blocks need to be
siblings, not nested. Take this Dart snippet:

try {
...
} on T1 catch (e1) {
   S1;
} on T2 catch (e2) {
  S2;
}

The way you nest blocks, variable e1 would be visible in the block containing
statement S2.

I think you can eliminate the need for the catch_blocks and type_tests array.
Create the Ast nodes as you parse, remember the previous SequenceNode of the
else-branch, and add the next if statement in the following loop iteration (or
the throw node after the loop terminates.)

[Kevin Millikin (Google), 2013-11-11 11:39:57]

Thanks for the review.  I think the scoping of the else blocks seems correct to
me and I can't seem to come up with a test that fails.

https://codereview.chromium.org/63983005/diff/140001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/63983005/diff/140001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6653: struct CatchParameter {
On 2013/11/08 21:52:30, hausner wrote:
> As I've aked in previous rounds of this review, please do not rename this.

CatchParamDesc is not a very good name.  First, what is a desc?  Description? 
Descriptor?  Probably not descendant.  The style guide is clear to 'do not use
abbreviations that are ambiguous...'.

But second, whatever desc is, it's a noise word.  It doesn't contribute anything
to the meaning of the identifier.  A distinction could be made between a
representation of a catch parameter and a representation of a
description/descriptor of a catch parameter but it's much too subtle to matter
here.  (And besides, how much more concrete would you expect this struct to be
for it to represent the parameter itself?)

https://codereview.chromium.org/63983005/diff/140001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6666: static bool AddCatchParamsToScope(CatchParameter*
exception_param,
On 2013/11/08 21:52:30, hausner wrote:
> Please leave this as a method of the parser and keep the error handling where
it
> is now.

Why?  The reason I moved it is so we could see the error handling code when
reading the caller, instead of buried in the helper.

For a helper with exactly one call site, it otherwise isn't especially vital
whether the caller or callee does the error handling, is it?

https://codereview.chromium.org/63983005/diff/140001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6910: current = CloseBlock();
On 2013/11/08 21:52:30, hausner wrote:
> The way you nest blocks, variable e1 would be visible in the block containing
> statement S2.

I don't think that's right.  The intent is to represent this as:

{
  if (:exception_var is T1) {
    var e1 = :exception_var;
    { S1; }
  } else if (:exception_var is T2) {
    var e2 = :exception-var;
    { S2; }
  }
}

I'll add a test.

> 
> I think you can eliminate the need for the catch_blocks and type_tests array.
> Create the Ast nodes as you parse, remember the previous SequenceNode of the
> else-branch, and add the next if statement in the following loop iteration (or
> the throw node after the loop terminates.)
> 

Let me see if I understand.  You suggest that though the nesting is actually:

if (test0) {
  parameters0
  { body0 }
} else {
  if (test1) {
    parameters1
    { body1 }
  }
}

I could parse it as if the block containing { parameters1 { body1 }} were not
really inside the else block (as far as the parser's current_block_ stack is
concerned)?  Is the reason that's safe because we know that this block doesn't
contain any bindings and is not labeled, and we don't use the current_block_
stack for anything else?  I was assuming that it was important to keep the
parser's current_block_ stack consistent with the AST we were building, but
maybe we already violate that in other places so I can go ahead and do it here.

If you think it's safe and want me to make this change, I will.

Note that the code as written is just a literal translation of the recursive
implementation with the first loop doing the work before the recursive
(non-tail) call, and the second loop doing the work after the return from the
recursive call.

[Kevin Millikin (Google), 2013-11-11 15:22:27]

This is changed to add unreachable catch bodies to the AST for the debugger. 
Matthias, any idea how I'd go about writing a test that a breakpoint in an
unreachable clause is not hit?  Can you point me toward a model?

I've restored AddCatchParamsToScope to the Parser class.  That makes me wonder
why it wants to be passed the scope (and the same question with
AddFormalParamsToScope) instead of using the parser's current_block_->scope?

[hausner, 2013-11-12 01:12:09]

I am not done looking at this change, will continue tomorrow. Unfortunately, you
insist in making completely unnecessary complications. It wish you'd just make
those changes that are necessary to eliminate the goto.

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6804: OpenBlock();
You are nesting 3 blocks now per case clause: one opened in line 6772, one in
6804 and one 6837 (ParseNestedStatement call).

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6845: // In the first cast, unconditionally execute the
catch body.  In the
case

[Kevin Millikin (Google), 2013-11-12 15:05:24]

On 2013/11/12 01:12:09, hausner wrote:
> Unfortunately, you
> insist in making completely unnecessary complications. It wish you'd just make
> those changes that are necessary to eliminate the goto.

You're really, really overstating this.

There are a small number of simple cleanups in this change.  The last I knew
cleaning up code we touched was still best practices (e.g., your change here
https://codereview.chromium.org/63033003/diff/80001/runtime/vm/parser.cc remains
a good one).

The cleanups are limited to:

1. I fixed broken includes.  If you think this is unnecessary complication, I'll
be happy to submit it as a separate change.

2. I factored about 150 lines out of the middle of a monolithic function so that
I could call them recursively.  Since I've now replaced that recursion with a
loop, I'll be happy to move those lines back into ParseTryStatement if you like
it better that way.  I do think having them in a separate function or otherwise
delimited makes it easier to read ParseTryStatement, but I don't insist.

3. Instead of (complicated :) ) data flow --- creating a local variable then
forgetting it only to look it up later in a (hopefully correct) scope --- I
remembered the variable when it was created and used it later. I think this is a
good change: now AddCatchParamsToScope is symmetric to AddFormalParamsToScope in
this way.

If you think that it's now more complicated than before, I'll revert this change
in the interest of getting this code in, and also make the corresponding change
to AddFormalParamsToScope for uniformity if you wish.

4. Otherwise, I renamed about a half dozen identifiers that were either
misspelled, actively confusing (e.g., the comments referred to one name, the
code used a completely different one, and I standardized), or just plain wrong
(e.g., the name indicated a type which was different from the type of the
variable).  There's nothing really complicated about simple renaming, anyway.


> You are nesting 3 blocks now per case clause: one opened in line 6772, one in
> 6804 and one 6837 (ParseNestedStatement call).

I know.  The outermost one of the three is because the outer CatchClauseNode
needs a SequenceNode (i.e., definitely not an IfNode) as its body and also
because the inner IfNodes need a SequenceNode for their else parts.  I think
this is actually a wart in the AST, but given that it's the case there will
necessarily be unnecessary blocks :).  Changing this is way out of scope of this
change.

The middle block holds the bindings of the catch parameters separate from the
body, and the innermost one represents the block of the catch body.

The outermost one can be eliminated for the last clause body if that body is
unconditional but that means looking ahead to see that it's the last one.  One
but not both of the two inner ones can be eliminated if either there are no
catch parameters or no bindings in the catch body.

Do you think it's worth trying to maximally eliminate these extra blocks?  It
doesn't seem like we go to extreme measures to avoid them anywhere else in the
parser, and while not free, empty blocks are pretty cheap.

[hausner, 2013-11-12 19:21:45]

I finally understand the nesting. LGTM with comments.

A couple of rounds back, you asked how to write a test to check that a
breakpoint in a dead case clause can be set but will never be reached. I think
it's not worth the effort to have a test for this, but if you still want to
create that, look for inspiration in tests/standalone/debugger.

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc
File runtime/vm/parser.cc (left):

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#old...
runtime/vm/parser.cc:7165: SourceLabel* label =
current_block_->scope->LookupInnermostCatchLabel();
I think you can eliminate the LookupInnermostCatchLabel function.

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6: 
Please keep unrelated changes separate. The include fixes belong in a separate
change.

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6653: struct CatchParameter {
Please do not rename CatchParamDesc. I understand you think it's an improvement,
but it's just a matter of taste. I prefer if it remains as is.

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6804: OpenBlock();
On 2013/11/12 01:12:10, hausner wrote:
> You are nesting 3 blocks now per case clause: one opened in line 6772, one in
> 6804 and one 6837 (ParseNestedStatement call).
> 

Ok, I get it now.

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6856: handler_types.SetLength(0);
Remember here with a bool variable that we've seen a catch clause that handles
all exceptions.

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6882: if (exception_param.type->IsInstantiated()) {
Also guard this with the boolean flag so that no more handler type entries are
added once we've seen a "catch all" clause.

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6893: if (type_tests.is_empty() ||
!type_tests.Last()->IsLiteralNode()) {
You can use the "catch all seen" flag to determine whether a re-throw is
necessary.

[Florian Schneider, 2013-11-13 10:25:41]

dbc:

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6: 
On 2013/11/12 19:21:45, hausner wrote:
> Please keep unrelated changes separate. The include fixes belong in a separate
> change.

I think we all agree changing includes to include only the necessary things and
avoid implicit hidden dependencies is a good thing: It saves compilation time
and prevents introducing accidental cyclic h-file dependencies.

I also think it is perfectly OK to do this changes "as you go". It does not make
the CL any more difficult to review and has no change in functionality. Nobody
want to waste cycles on extra CLs to clean up header-includes if not absolutely
necessary. So +1 from my side for small delta improvements like this.

[Kevin Millikin (Google), 2013-11-13 13:47:02]

Matthias, thanks for the review.

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc
File runtime/vm/parser.cc (left):

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#old...
runtime/vm/parser.cc:7165: SourceLabel* label =
current_block_->scope->LookupInnermostCatchLabel();
On 2013/11/12 19:21:45, hausner wrote:
> I think you can eliminate the LookupInnermostCatchLabel function.

Yes, thank you.  Done.

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6653: struct CatchParameter {
On 2013/11/12 19:21:45, hausner wrote:
> Please do not rename CatchParamDesc. I understand you think it's an
improvement,
> but it's just a matter of taste. I prefer if it remains as is. 

I changed it back, but it's not *just* a matter of taste.  'Desc' is ambiguous,
because it's not obvious from the name if it's a description or a descriptor. 
Those two things are nearly opposites.

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6845: // In the first cast, unconditionally execute the
catch body.  In the
On 2013/11/12 01:12:10, hausner wrote:
> case

Done.

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6856: handler_types.SetLength(0);
On 2013/11/12 19:21:45, hausner wrote:
> Remember here with a bool variable that we've seen a catch clause that handles
> all exceptions.

Done.

https://codereview.chromium.org/63983005/diff/240002/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:6882: if (exception_param.type->IsInstantiated()) {
On 2013/11/12 19:21:45, hausner wrote:
> Also guard this with the boolean flag so that no more handler type entries are
> added once we've seen a "catch all" clause.

Done.

[Kevin Millikin (Google), 2013-11-13 14:08:23]

Committed patchset #5 manually as r30227 (presubmit successful).