Page MenuHomePhabricator

Improved error recovery for _Pragma
ClosedPublic

Authored by rcraik on Mar 12 2015, 4:14 PM.

Details

Summary

Currently, if the argument to _Pragma is not a parenthesised string literal, the bad token will be consumed, as well as the ')', if present. If additional bad tokens are passed to the _Pragma, this results in extra error messages which may distract from the true problem. For example:

temp.c:1:1: error: _Pragma takes a parenthesized string literal
_Pragma(unroll 1)
^
temp.c:1:16: error: expected identifier or '('
_Pragma(unroll 1)
               ^
2 errors generated.
}

The proposed patch causes all tokens to be consumed until the closing ')' or a new line, whichever is reached first.

Diff Detail

Event Timeline

rcraik updated this revision to Diff 21873.Mar 12 2015, 4:14 PM
rcraik retitled this revision from to Improved error recovery for _Pragma.
rcraik updated this object.
rcraik edited the test plan for this revision. (Show Details)
rcraik added reviewers: rsmith, hfinkel.
rcraik added subscribers: Unknown Object (MLST), rnk, fraggamuffin.

The old code would have handled this okay:

_Pragma(
BAD_TOKEN
)

At the same time, the case could have looked like this:

_Pragma( void bar() {
  foo();
}
/**EOF**/

Both of these could be handled by using a matched parentheses approach: it eats the _Pragma in the first, and erases the rest of the file in the second (avoiding spurious messages following the _Pragma).

rcraik added a comment.EditedMar 13 2015, 10:57 AM

In regards to the previous comment, a BalancedDelimiterTracker could (in theory) be used instead to eat all tokens until the closing ')', if it exists. However, _Pragma is handled by the preprocessor, and I believe BalancedDelimiterTracker can only used from within the parser.

With this patch I have tried to follow the style used elsewhere in the preprocessor.

rcraik updated this revision to Diff 21979.Mar 13 2015, 7:29 PM

I've updated the patch to address the first case mentioned by Hubert. The behaviour with this patch will now be the same as the current trunk

rsmith accepted this revision.Jul 29 2015, 8:01 PM
rsmith edited edge metadata.

LGTM

This looks like a strict improvement on what we did before, even though it doesn't perfectly handle all of Hubert's testcases.

This revision is now accepted and ready to land.Jul 29 2015, 8:01 PM