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.
================
Comment at: include/clang/Basic/DiagnosticLexKinds.td:463-464
@@ -462,1 +462,4 @@
"too many arguments provided to function-like macro invocation">;
+def note_suggest_parens_for_macro : Note<
+ "braced lists require parenthesis to be recognized inside macros">;
+def note_cannot_use_init_list_as_macro_argument : Note<
----------------
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"?
================
Comment at: lib/Lex/PPMacroExpansion.cpp:447
@@ +446,3 @@
+ // Checks that braces and parens are properly nested. If not, return.
+ SmallVector<bool, 8> IsParen;
+ for (SmallVector<Token, 64>::iterator TokenIterator = MacroTokens.begin(),
----------------
Using an enum for paren/brace instead of bool here would be clearer.
================
Comment at: lib/Lex/PPMacroExpansion.cpp:451-463
@@ +450,15 @@
+ TokenIterator != TokenEnd; ++TokenIterator) {
+ if (TokenIterator->is(tok::l_paren)) {
+ IsParen.push_back(true);
+ } else if (TokenIterator->is(tok::r_paren)) {
+ if (IsParen.empty() || !IsParen.back())
+ return 0;
+ IsParen.pop_back();
+ } else if (TokenIterator->is(tok::l_brace)) {
+ IsParen.push_back(false);
+ } else if (TokenIterator->is(tok::r_brace)) {
+ if (IsParen.empty() || IsParen.back())
+ return 0;
+ IsParen.pop_back();
+ }
+ }
----------------
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.
================
Comment at: lib/Lex/PPMacroExpansion.cpp:500
@@ +499,3 @@
+ // End of argument.
+ if (!FoundComma || !FoundBrace) {
+ CorrectedMacroTokens.insert(CorrectedMacroTokens.end(),
----------------
Do you need the FoundBrace check here? It looks like FoundComma implies
FoundBrace, so you can remove the FoundBrace variable entirely.
================
Comment at: lib/Lex/PPMacroExpansion.cpp:469
@@ +468,3 @@
+ unsigned Braces = 0;
+ bool FoundComma = false;
+ bool FoundBrace = false;
----------------
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.
================
Comment at: lib/Lex/PPMacroExpansion.cpp:503-506
@@ +502,6 @@
+ ArgStart, TokenIterator);
+ } else if (ArgStart->is(tok::l_brace) &&
+ (TokenIterator - 1)->is(tok::r_brace)) {
+ InitLists.push_back(SourceRange(ArgStart->getLocation(),
+ (TokenIterator - 1)->getLocation()));
+ } else {
----------------
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?
================
Comment at: lib/Lex/PPMacroExpansion.cpp:510-513
@@ +509,6 @@
+ TokenIterator->getLocation()));
+ CorrectedMacroTokens.push_back(MacroTokens.front());
+ CorrectedMacroTokens.insert(CorrectedMacroTokens.end(),
+ ArgStart, TokenIterator);
+ CorrectedMacroTokens.push_back(MacroTokens.back());
+ }
----------------
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)?
================
Comment at: lib/Lex/PPMacroExpansion.cpp:512
@@ +511,3 @@
+ CorrectedMacroTokens.insert(CorrectedMacroTokens.end(),
+ ArgStart, TokenIterator);
+ CorrectedMacroTokens.push_back(MacroTokens.back());
----------------
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 ).
================
Comment at: lib/Lex/PPMacroExpansion.cpp:503-504
@@ +502,4 @@
+ ArgStart, TokenIterator);
+ } else if (ArgStart->is(tok::l_brace) &&
+ (TokenIterator - 1)->is(tok::r_brace)) {
+ InitLists.push_back(SourceRange(ArgStart->getLocation(),
----------------
Richard Smith wrote:
> 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?
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.
http://llvm-reviews.chandlerc.com/D986
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
