This is an archive of the discontinued LLVM Phabricator instance.

Better macro error messages involving initializer lists
ClosedPublic

Authored by rtrieu on Jun 14 2013, 7:27 PM.

Details

Reviewers
rsmith
Summary

When an initializer list is used within a marco argument, each comma is the list is treated as a macro argument separator. When this happens, the only error message is "too many macro arguments provided". This patch changes the handling of function-like macros to give additional notes detailing the problems.

For a macro FOO(vector<int>{1,2,3}, 3), it will suggest parenthesis to preserve the initializer list with the corrected code as FOO((vector<int>{1,2,3}), 3).

For a macro such as FOO( {1,2,3} ), it will provide a different note saying that initializer lists cannot be used as macro arguments. This is because adding parentheses around the braces would make it no longer an initializer list.

Diff Detail

Event Timeline

rsmith added a comment.Jul 1 2013, 4:33 PM

This looks essentially reasonable (assuming the extra Token copying doesn't cause any measurable slowdown for preprocessor-intensive code -- there are some boost libraries that might be worth checking here).

You need to rebase this patch; it will conflict with the MS ignored commas patch which landed last week.

include/clang/Basic/DiagnosticLexKinds.td
463–464

This is ambiguous, and could be read as "braced lists require that parentheses are recognized inside macros". Maybe "parentheses are required around macro argument containing braced initializer list"?

lib/Lex/PPMacroExpansion.cpp
582

Using an enum for paren/brace instead of bool here would be clearer.

586–598

Have you considered trying to also match angle-brackets if matching braces doesn't work? That would be useful for a number of template cases.

604

Can you make this variable name more descriptive or add a comment? Something which more explicitly says that this tracks whether the current corrected argument includes a braced but unparenthesized comma.

635

Do you need the FoundBrace check here? It looks like FoundComma implies FoundBrace, so you can remove the FoundBrace variable entirely.

638–639

The parens fix-up won't work in any case where the ArgStart is l_brace, no matter what the end token is. It's a shame that we don't recover well in this case; it'd be nice to treat the argument as a single item regardless, but the current factoring of the code makes that very hard.

Have you considered making this code directly produce MacroArgs instead (maybe performing these checks as a fixup after the handling in ProcessFunctionLikeMacroArgTokens), so that you can produce arguments with embedded commas? Alternatively, we now have the Token::IgnoredComma flag that you can use to indicate that a comma is not a macro argument separator; you could use that for recovery.

638–641

This note isn't appropriate if the argument is of the form "{...} stuff {...}". Perhaps you shouldn't give a note at all for that case?

645–648

Reusing MacroTokens.front() and MacroTokens.back() here will give surprising source locations. Perhaps put the ( at the location of the first token, and put the ) past the end of the location of the last token instead (that is, at the locations where you point the fix-its)?

647

The TokenIterator location here should really be the location past the end of the prior token. Imagine we had:

MACRO(
  vector<int>{1, 2, 3}
)

We'd want the ) suggested after the }, not before the ).

rtrieu updated this revision to Unknown Object (????).Jul 19 2013, 2:41 PM

Refactoring the earlier patch to take into account comments.

  • Instead of attempting a correction after every failure, only try after a "too many arguments" failure.
  • Rewrite the code that generates a new vector of token arguments to use the old vector of tokens. This cuts back the amount of token copying to only when a correction is attempted.
  • Give a note on all arguments started with a brace list instead of only when the brace list is the entire token.
rtrieu added inline comments.Jul 19 2013, 2:48 PM
include/clang/Basic/DiagnosticLexKinds.td
463–464

Done.

lib/Lex/PPMacroExpansion.cpp
582

Done.

586–598

Scope creep! Maybe in a future patch. Added a TODO for now.

604

Renamed and added comments.

638–639

The new patch intercepts the argument tokens earlier, so only one attempt to generate MacroArgs is attempted.

Arguments starting with an l_brace now produce the "cannot start with a braced list" note.

645–648

Now using the locations of the beginning and end tokens of the argument.

647

Using getLocForEndOfToken() as needed.

rsmith added inline comments.Jul 19 2013, 3:17 PM
lib/Lex/PPMacroExpansion.cpp
443

This variable seems to be unused.

482

Incomplete comment.

693–696

It looks like DiagnosticBuilder will assert if given more than 10 ranges. Maybe guard this with DB.hasMaxRanges()?

704–709

Likewise w/fixits.

test/Preprocessor/macro_with_initializer_list.cpp
141

This is done, right?

rtrieu updated this revision to Unknown Object (????).Jul 19 2013, 5:37 PM

Also fixed a problem where the too many arguments error was always pointing to the last argument. It now properly points to the first extra element.

lib/Lex/PPMacroExpansion.cpp
443

Variable removed.

482

Done.

693–696

Done with test case.

704–709

Done with test case.

test/Preprocessor/macro_with_initializer_list.cpp
141

Yes.

rsmith accepted this revision.Jul 22 2013, 4:02 PM

LGTM with a couple of tiny tweaks.

include/clang/Basic/Diagnostic.h
987

Should be "hasMaxFixItHints" presumably?

lib/Lex/PPMacroExpansion.cpp
710–713

For robustness, please check hasMaxFixItHints between adding the two fixits here. The maximum might not always be even.

rtrieu closed this revision.Jul 23 2013, 11:07 AM

Committed with tiny tweaks at r186971