[PATCH] Better macro error messages involving initializer lists

[Richard Smith]

  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