[cfe-commits] [PATCH] Suppress elided variadic macro argument diagnostic with comma pasting extension

[Eli Friedman]

Suppose you have a testcase like the following:

#define NSAssert(condition, desc, ...) \
    SomeComplicatedStuff((desc), ##__VA_ARGS__)
NSAssert(somecond, somedesc)

We currently print two diagnostics:

<stdin>:3:28: warning: must specify at least one argument for '...'
parameter of variadic macro [-Wgnu]
NSAssert(somecond, somedesc)
                           ^
<stdin>:1:9: note: macro 'NSAssert' defined here
#define NSAssert(condition, desc, ...) \
        ^
<stdin>:2:32: warning: token pasting of ',' and __VA_ARGS__ is a GNU
extension [-Wgnu]
    SomeComplicatedStuff((desc), ##__VA_ARGS__)


The attached patch suppresses the first diagnostic in certain cases
where we know we're going to print the second diagnostic.  The
diagnostic is redundant, and it can't be suppressed in the definition
of the macro because it points at the use of the macro, so we want to
avoid printing it if possible.

The implementation works by detecting constructs which look like comma
pasting at the time of the definition of the macro; this information
is then used when the macro is used.  (We can't actually detect
whether we're using the comma pasting extension until the macro is
actually used, but we can detecting constructs which will be comma
pasting if the varargs argument is elided.)

I'm asking for a review because I'm not very familiar with this code.

-Eli
-------------- next part --------------
Index: test/Preprocessor/macro_fn.c
===================================================================
--- test/Preprocessor/macro_fn.c	(revision 167645)
+++ test/Preprocessor/macro_fn.c	(working copy)
@@ -44,3 +44,9 @@
 #define E() (i == 0)
 #if E
 #endif
+
+
+/* <rdar://problem/12292192> */
+#define NSAssert(condition, desc, ...) /* expected-warning {{variadic macros are a C99 feature}} */ \
+    SomeComplicatedStuff((desc), ##__VA_ARGS__) /* expected-warning {{token pasting of ',' and __VA_ARGS__ is a GNU extension}} */
+NSAssert(somecond, somedesc)
Index: include/clang/Lex/MacroInfo.h
===================================================================
--- include/clang/Lex/MacroInfo.h	(revision 167645)
+++ include/clang/Lex/MacroInfo.h	(working copy)
@@ -76,6 +76,9 @@
   /// it has not yet been redefined or undefined.
   bool IsBuiltinMacro : 1;
 
+  /// \brief Whether this macro contains the sequence ", ## __VA_ARGS__"
+  bool HasCommaPasting : 1;
+
   /// \brief True if this macro was loaded from an AST file.
   bool IsFromAST : 1;
 
@@ -253,6 +256,9 @@
   /// __LINE__, which requires processing before expansion.
   bool isBuiltinMacro() const { return IsBuiltinMacro; }
 
+  bool hasCommaPasting() const { return HasCommaPasting; }
+  void setHasCommaPasting() { HasCommaPasting = true; }
+
   /// isFromAST - Return true if this macro was loaded from an AST file.
   bool isFromAST() const { return IsFromAST; }
 
Index: lib/Lex/PPDirectives.cpp
===================================================================
--- lib/Lex/PPDirectives.cpp	(revision 167645)
+++ lib/Lex/PPDirectives.cpp	(working copy)
@@ -1809,7 +1809,7 @@
     while (Tok.isNot(tok::eod)) {
       LastTok = Tok;
 
-      if (Tok.isNot(tok::hash)) {
+      if (Tok.isNot(tok::hash) && Tok.isNot(tok::hashhash)) {
         MI->AddTokenToBody(Tok);
 
         // Get the next token of the macro.
@@ -1817,6 +1817,35 @@
         continue;
       }
 
+      if (Tok.is(tok::hashhash)) {
+        
+        // If we see token pasting, check if it looks like the gcc comma
+        // pasting extension.  We'll use this information to suppress
+        // diagnostics later on.
+        
+        // Get the next token of the macro.
+        LexUnexpandedToken(Tok);
+
+        if (Tok.is(tok::eod)) {
+          MI->AddTokenToBody(LastTok);
+          break;
+        }
+
+        unsigned NumTokens = MI->getNumTokens();
+        if (NumTokens && Tok.getIdentifierInfo() == Ident__VA_ARGS__ &&
+            MI->getReplacementToken(NumTokens-1).is(tok::comma))
+          MI->setHasCommaPasting();
+
+        // Things look ok, add the '##' and param name tokens to the macro.
+        MI->AddTokenToBody(LastTok);
+        MI->AddTokenToBody(Tok);
+        LastTok = Tok;
+
+        // Get the next token of the macro.
+        LexUnexpandedToken(Tok);
+        continue;
+      }
+
       // Get the next token of the macro.
       LexUnexpandedToken(Tok);
 
Index: lib/Lex/MacroInfo.cpp
===================================================================
--- lib/Lex/MacroInfo.cpp	(revision 167645)
+++ lib/Lex/MacroInfo.cpp	(working copy)
@@ -25,6 +25,7 @@
     IsC99Varargs(false),
     IsGNUVarargs(false),
     IsBuiltinMacro(false),
+    HasCommaPasting(false),
     IsFromAST(false),
     ChangedAfterLoad(false),
     IsDisabled(false),
@@ -50,6 +51,7 @@
     IsC99Varargs(MI.IsC99Varargs),
     IsGNUVarargs(MI.IsGNUVarargs),
     IsBuiltinMacro(MI.IsBuiltinMacro),
+    HasCommaPasting(MI.HasCommaPasting),
     IsFromAST(MI.IsFromAST),
     ChangedAfterLoad(MI.ChangedAfterLoad),
     IsDisabled(MI.IsDisabled),
Index: lib/Lex/PPMacroExpansion.cpp
===================================================================
--- lib/Lex/PPMacroExpansion.cpp	(revision 167645)
+++ lib/Lex/PPMacroExpansion.cpp	(working copy)
@@ -619,9 +619,14 @@
       // Varargs where the named vararg parameter is missing: OK as extension.
       //   #define A(x, ...)
       //   A("blah")
-      Diag(Tok, diag::ext_missing_varargs_arg);
-      Diag(MI->getDefinitionLoc(), diag::note_macro_here)
-        << MacroName.getIdentifierInfo();
+      //
+      // If the macro contains the comma pasting extension, the diagnostic
+      // is suppressed; we know we'll get another diagnostic later.
+      if (!MI->hasCommaPasting()) {
+        Diag(Tok, diag::ext_missing_varargs_arg);
+        Diag(MI->getDefinitionLoc(), diag::note_macro_here)
+          << MacroName.getIdentifierInfo();
+      }
 
       // Remember this occurred, allowing us to elide the comma when used for
       // cases like:
Index: lib/Serialization/ASTWriter.cpp
===================================================================
--- lib/Serialization/ASTWriter.cpp	(revision 167645)
+++ lib/Serialization/ASTWriter.cpp	(working copy)
@@ -1855,6 +1855,7 @@
 
         Record.push_back(MI->isC99Varargs());
         Record.push_back(MI->isGNUVarargs());
+        Record.push_back(MI->hasCommaPasting());
         Record.push_back(MI->getNumArgs());
         for (MacroInfo::arg_iterator I = MI->arg_begin(), E = MI->arg_end();
              I != E; ++I)
Index: lib/Serialization/ASTReader.cpp
===================================================================
--- lib/Serialization/ASTReader.cpp	(revision 167645)
+++ lib/Serialization/ASTReader.cpp	(working copy)
@@ -1141,6 +1141,7 @@
         // Decode function-like macro info.
         bool isC99VarArgs = Record[NextIndex++];
         bool isGNUVarArgs = Record[NextIndex++];
+        bool hasCommaPasting = Record[NextIndex++];
         MacroArgs.clear();
         unsigned NumArgs = Record[NextIndex++];
         for (unsigned i = 0; i != NumArgs; ++i)
@@ -1150,6 +1151,7 @@
         MI->setIsFunctionLike();
         if (isC99VarArgs) MI->setIsC99Varargs();
         if (isGNUVarArgs) MI->setIsGNUVarargs();
+        if (hasCommaPasting) MI->setHasCommaPasting();
         MI->setArgumentList(MacroArgs.data(), MacroArgs.size(),
                             PP.getPreprocessorAllocator());
       }