[PATCH] Better macro error messages involving initializer lists

[Richard Trieu]

================
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<
----------------
Richard Smith wrote:
> 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"?
Done.

================
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(),
----------------
Richard Smith wrote:
> Using an enum for paren/brace instead of bool here would be clearer.
Done.

================
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();
+    }
+  }
----------------
Richard Smith wrote:
> 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.
Scope creep!  Maybe in a future patch.  Added a TODO for now.

================
Comment at: lib/Lex/PPMacroExpansion.cpp:469
@@ +468,3 @@
+  unsigned Braces = 0;
+  bool FoundComma = false;
+  bool FoundBrace = false;
----------------
Richard Smith wrote:
> 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.
Renamed and added comments.

================
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:
> 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.
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.

================
Comment at: lib/Lex/PPMacroExpansion.cpp:512
@@ +511,3 @@
+        CorrectedMacroTokens.insert(CorrectedMacroTokens.end(),
+                                    ArgStart, TokenIterator);
+        CorrectedMacroTokens.push_back(MacroTokens.back());
----------------
Richard Smith wrote:
> 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 ).
Using getLocForEndOfToken() as needed.

================
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());
+      }
----------------
Richard Smith wrote:
> 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)?
Now using the locations of the beginning and end tokens of the argument.


http://llvm-reviews.chandlerc.com/D986