This is an archive of the discontinued LLVM Phabricator instance.

Consolidate and improve the handling of built-in feature-like macros
ClosedPublic

Authored by AndyG on Feb 11 2016, 9:13 AM.

Details

Summary

The parsing logic has been separated out from the macro implementation logic, leading to a number of improvements:

  • Gracefully handle unexpected/invalid tokens, too few, too many and nested parameters
  • Provide consistent behaviour between all built-in feature-like macros
  • Simplify the implementation of macro logic
  • Fix __is_identifier to correctly return '0' for non-identifiers

Diff Detail

Event Timeline

AndyG updated this revision to Diff 47665.Feb 11 2016, 9:13 AM
AndyG retitled this revision from to Consolidate and improve the handling of built-in feature-like macros.
AndyG updated this object.
AndyG updated this object.Feb 11 2016, 9:19 AM
AndyG added a reviewer: doug.gregor.
AndyG added a subscriber: cfe-commits.

Test comment -- just to see whether email notifications are sending properly (it seems, for example, that cfe-commits wasn't notified of this patch...)

rsmith added a subscriber: rsmith.Feb 11 2016, 5:24 PM
rsmith added inline comments.
lib/Lex/PPMacroExpansion.cpp
1451–1467

Why should this be treated as valid? That's not part of the specification for these features.

To be honest, the simple answer is because it was just as easy to do with nesting as without (the code would still need to track the appearance of left and right parentheses in order to correctly parse to the closing right-parenthesis of the macro invocation in any case).

And since the work has to be done anyway, it seemed reasonable to allow it. My other thought was that when people write wrapper macros, they often (indeed idiomatically) wrap their parameters in an extra set of parentheses before passing down to the next level. This would therefore handle this use-case.

While it may not technically be part of the specification, neither does it break it, and in the 99% most common cases the extra functionality will not come into play.

The motivation of the patch was to ensure a more robust parsing of these feature-like macros -- that and fixing __is_identifier which was fundamentally broken and where I originally started.

If you really disagree with this extension of the rules noted in your comment, I can work a logic that errors on the presence of embedded parentheses, but IMHO I think it preferable to keep it as is :o)

AndyG added a comment.Feb 25 2016, 6:37 AM

Second bump :o)

AndyG updated this revision to Diff 52513.Apr 3 2016, 2:07 PM

Ok, I've removed support for nested parentheses. Can this go through now?

Thanks.

rsmith added inline comments.Apr 3 2016, 5:12 PM
lib/Lex/PPMacroExpansion.cpp
1430–1432

Use llvm::function_ref here instead, and don't pass a Preprocessor into the function (instead, capture it in a lambda).

1456–1457

If we get an annotation token here, we should reject it, not silently ignore it. Also, we shouldn't see comment tokens here (we shouldn't be doing macro expansion with comments enabled); you should call LexUnexpandedToken rather than LexUnexpandedNonComment.

1481–1484

The only way we can get here without already having a value or producing a diagnostic is if this is the first token inside the parens. So this will always say "unexpected '(' after '('".

I think it would be better to always break here after incrementing ParenDepth (even when !Result.hasValue()), and let Op produce the relevant diagnostic for this case.

1519

expected -> missing

AndyG updated this revision to Diff 52535.Apr 4 2016, 2:05 AM
AndyG marked 5 inline comments as done.

Implemented comments.

lib/Lex/PPMacroExpansion.cpp
1456–1457

Ok, annotation tokens are not ignored, but drop down into Op and can be handled there.

1481–1484

I've handled this via a specific diagnostic message: "nested parentheses not permitted in %0" where %0 is the macro name. I would prefer this to bringing the error checking into Op since this would complicate the implementation of Op needlessly IMHO. Op should be as slimline as possible.

rsmith accepted this revision.Apr 4 2016, 1:06 PM
rsmith added a reviewer: rsmith.
rsmith added inline comments.
lib/Lex/PPMacroExpansion.cpp
1481–1484

I think this can be done without any changes to any Op implementation: each of them already diagnoses a left paren just as it would diagnose any other unexpected token. But the custom diagnostic also seems fine.

This revision is now accepted and ready to land.Apr 4 2016, 1:06 PM
This revision was automatically updated to reflect the committed changes.