[Preprocessor] Improve MSVC comma-elision before an empty expansion.

[erichkeane]

Summary

We correctly implemented when VA_ARGS expanded to nothing, however a macro that is an empty expansion ALSO omits the comma (with a few small infuriating examples). This patch strives to correct the rest of the behaviors.

[erichkeane]

This has likely gone stale, and my employer has decided to tell customers to modify their code.

include/clang/Lex/Token.h

class Token {
   /// Flags - Bits we track about this token, members of the TokenFlags enum.
   unsigned short Flags;
 
 public:
   // Various flags set per token:
   enum TokenFlags {
     StartOfLine   = 0x01,  // At start of line or only after whitespace
                            // (considering the line after macro expansion).
-    LeadingSpace  = 0x02,  // Whitespace exists before this token (considering 
+    LeadingSpace  = 0x02,  // Whitespace exists before this token (considering
                            // whitespace after macro expansion).
     DisableExpand = 0x04,  // This identifier may never be macro expanded.
     NeedsCleaning = 0x08,  // Contained an escaped newline or trigraph.
     LeadingEmptyMacro = 0x10, // Empty macro exists before this token.
     HasUDSuffix = 0x20,    // This string or character literal has a ud-suffix.
     HasUCN = 0x40,         // This identifier contains a UCN.
     IgnoredComma = 0x80,   // This comma is not a macro argument separator (MS).
     StringifiedInMacro = 0x100, // This string or character literal is formed by
                                 // macro stringizing or charizing operator.
     CommaAfterElided = 0x200, // The comma following this token was elided (MS).
+    CommaBeforeElided = 0x400, // The comma before this token was elided, used
+                               // in conjunction with below (MS).
+    CommaBeforeNeedsElision = 0x800, // A comma in a Macro before a __VA_ARGS__
+                                     // expansion shouldn't be emitted, EVEN if
+                                     // it is hidden in in a function like macro.
+                                     // This marks the token if that condition
+                                     // has yet to be satisified(MS).
   };
 
   tok::TokenKind getKind() const { return Kind; }
   void setKind(tok::TokenKind K) { Kind = K; }
 
   /// is/isNot - Predicates to check if this token is a specific kind, as in
   /// "if (Tok.is(tok::l_brace)) {...}".
   bool is(tok::TokenKind K) const { return Kind == K; }

include/clang/Lex/TokenLexer.h

class TokenLexer {
   /// macros (for example) we just point into the token buffer of the macro
   /// definition, we don't make a copy of it.
   bool OwnsTokens : 1;
 
   /// DisableMacroExpansion - This is true when tokens lexed from the TokenLexer
   /// should not be subject to further macro expansion.
   bool DisableMacroExpansion : 1;
 
+  /// UnfulfilledCommaElision - This is true when the TokenLexer has identified
+  /// an empty __VA_ARGS__ item but has been unable to elide the comma for it or
+  /// mark a token for comma elision.  This likely happens when this TokenLexer
+  /// represents an empty function macro, like M(...) __VA_ARGS__.  This is used
+  /// later to apply this value to the next available token.
+  bool UnfulfilledCommaElision : 1;
+
   TokenLexer(const TokenLexer &) = delete;
   void operator=(const TokenLexer &) = delete;
 public:
   /// Create a TokenLexer for the specified macro with the specified actual
   /// arguments.  Note that this ctor takes ownership of the ActualArgs pointer.
   /// ILEnd specifies the location of the ')' for a function-like macro or the
   /// identifier for an object-like macro.
   TokenLexer(Token &Tok, SourceLocation ILEnd, MacroInfo *MI,
              MacroArgs *ActualArgs, Preprocessor &pp)
-    : Macro(nullptr), ActualArgs(nullptr), PP(pp), OwnsTokens(false) {
+      : Macro(nullptr), ActualArgs(nullptr), PP(pp), OwnsTokens(false),
+        UnfulfilledCommaElision(false) {
     Init(Tok, ILEnd, MI, ActualArgs);
   }
 
   /// Init - Initialize this TokenLexer to expand from the specified macro
   /// with the specified argument information.  Note that this ctor takes
   /// ownership of the ActualArgs pointer.  ILEnd specifies the location of the
   /// ')' for a function-like macro or the identifier for an object-like macro.
   void Init(Token &Tok, SourceLocation ILEnd, MacroInfo *MI,
             MacroArgs *ActualArgs);
 
   /// Create a TokenLexer for the specified token stream.  If 'OwnsTokens' is
   /// specified, this takes ownership of the tokens and delete[]'s them when
   /// the token lexer is empty.
   TokenLexer(const Token *TokArray, unsigned NumToks, bool DisableExpansion,
              bool ownsTokens, Preprocessor &pp)
-    : Macro(nullptr), ActualArgs(nullptr), PP(pp), OwnsTokens(false) {
+      : Macro(nullptr), ActualArgs(nullptr), PP(pp), OwnsTokens(false),
+        UnfulfilledCommaElision(false) {
     Init(TokArray, NumToks, DisableExpansion, ownsTokens);
   }
 
   /// Init - Initialize this TokenLexer with the specified token stream.
   /// This does not take ownership of the specified token vector.
   ///
   /// DisableExpansion is true when macro expansion of tokens lexed from this
   /// stream should be disabled.

lib/Lex/PPMacroExpansion.cpp

 MacroArgs *Preprocessor::ReadFunctionLikeMacroArgs(Token &MacroName,
                                                    MacroInfo *MI,
                                                    SourceLocation &MacroEnd) {
   // The number of fixed arguments to parse.
   unsigned NumFixedArgsLeft = MI->getNumArgs();
   bool isVariadic = MI->isVariadic();
 
   // Outer loop, while there are more arguments, keep reading them.
-  Token Tok;
+  Token Tok{};
 
   // Read arguments as unexpanded tokens.  This avoids issues, e.g., where
   // an argument value in a macro could expand to ',' or '(' or ')'.
   LexUnexpandedToken(Tok);
   assert(Tok.is(tok::l_paren) && "Error computing l-paren-ness?");
 
   // ArgTokens - Build up a list of tokens that make up each argument.  Each
   // argument is separated by an EOF token.  Use a SmallVector so we can avoid

lib/Lex/TokenLexer.cpp

void TokenLexer::Init(Token &Tok, SourceLocation ELEnd, MacroInfo *MI,
   ExpandLocStart = Tok.getLocation();
   ExpandLocEnd = ELEnd;
   AtStartOfLine = Tok.isAtStartOfLine();
   HasLeadingSpace = Tok.hasLeadingSpace();
   NextTokGetsSpace = false;
   Tokens = &*Macro->tokens_begin();
   OwnsTokens = false;
   DisableMacroExpansion = false;
-  NumTokens = Macro->tokens_end()-Macro->tokens_begin();
+  UnfulfilledCommaElision = false;
+  NumTokens = Macro->tokens_end() - Macro->tokens_begin();
   MacroExpansionStart = SourceLocation();
 
   SourceManager &SM = PP.getSourceManager();
   MacroStartSLocOffset = SM.getNextLocalOffset();
 
   if (NumTokens > 0) {
     assert(Tokens[0].getLocation().isValid());
     assert((Tokens[0].getLocation().isFileID() || Tokens[0].is(tok::comment)) &&
void TokenLexer::Init(const Token *TokArray, unsigned NumToks,
   // associated with it.
   destroy();
 
   Macro = nullptr;
   ActualArgs = nullptr;
   Tokens = TokArray;
   OwnsTokens = ownsTokens;
   DisableMacroExpansion = disableMacroExpansion;
+  UnfulfilledCommaElision = false;
   NumTokens = NumToks;
   CurToken = 0;
   ExpandLocStart = ExpandLocEnd = SourceLocation();
   AtStartOfLine = false;
   HasLeadingSpace = false;
   NextTokGetsSpace = false;
   MacroExpansionStart = SourceLocation();
 
bool TokenLexer::MaybeRemoveCommaBeforeVaArgs(
   // __VA_ARGS__ is empty, but not in strict C99 mode where there are no
   // named arguments, where it remains.  In all other modes, including C99
   // with GNU extensions, it is removed regardless of named arguments.
   // Microsoft also appears to support this extension, unofficially.
   if (PP.getLangOpts().C99 && !PP.getLangOpts().GNUMode
         && Macro->getNumArgs() < 2)
     return false;
 
+  // if we SHOULD have erased a comma, but we don't have any place to do so,
+  // mark the TokenLexer to consider doing so later.
+  if (ResultToks.empty() && !HasPasteOperator) {
+    UnfulfilledCommaElision = true;
+    return false;
+  }
+
   // Is a comma available to be removed?
   if (ResultToks.empty() || !ResultToks.back().is(tok::comma))
     return false;
 
   // Issue an extension diagnostic for the paste operator.
   if (HasPasteOperator)
     PP.Diag(ResultToks.back().getLocation(), diag::ext_paste_comma);
 
if (!PasteBefore && ActualArgs->isVarargsElidedUse() &&
                                      /*HasPasteOperator=*/false,
                                      Macro, ArgNo, PP))
       continue;
 
     // If it is not the LHS/RHS of a ## operator, we must pre-expand the
     // argument and substitute the expanded tokens into the result.  This is
     // C99 6.10.3.1p1.
     if (!PasteBefore && !PasteAfter) {
-      ExpandSingleFunctionArgument(CurTok, ArgNo, ResultToks);
+      if (!ExpandSingleFunctionArgument(CurTok, ArgNo, ResultToks))
+        MaybeRemoveCommaBeforeVaArgs(ResultToks,
+                                     /*HasPasteOperator=*/false, Macro, ArgNo,
+                                     PP);
       continue;
     }
 
     // Okay, we have a token that is either the LHS or RHS of a paste (##)
     // argument.  It gets substituted as its non-pre-expanded tokens.
     const Token *ArgToks = ActualArgs->getUnexpArgument(ArgNo);
     unsigned NumToks = MacroArgs::getArgLength(ArgToks);
     if (NumToks) {  // Not an empty argument?
for (unsigned i = 0, e = NumTokens; i != e; ++i) {
     // the ## was a comma, remove the comma.  This is a GCC extension which is
     // disabled when using -std=c99.
     if (ActualArgs->isVarargsElidedUse())
       MaybeRemoveCommaBeforeVaArgs(ResultToks,
                                    /*HasPasteOperator=*/true,
                                    Macro, ArgNo, PP);
   }
 
+  // If we have a leading empty expansion that we were unable to remove a comma,
+  // mark the 'front' token for removal so that it can be removed later.
+  if (!ResultToks.empty() && UnfulfilledCommaElision) {
+    ResultToks.front().setFlag(Token::CommaBeforeNeedsElision);
+    UnfulfilledCommaElision = false;
+  }
+
   // If anything changed, install this as the new Tokens list.
   if (MadeChange) {
     assert(!OwnsTokens && "This would leak if we already own the token list");
     // This is deleted in the dtor.
     NumTokens = ResultToks.size();
     // The tokens will be added to Preprocessor's cache and will be removed
     // when this TokenLexer finishes lexing them.
     Tokens = PP.cacheMacroExpandedTokens(this, ResultToks);
if (isAtEnd()) {
     // that it is no longer being expanded.
     if (Macro) Macro->EnableMacro();
 
     Tok.startToken();
     Tok.setFlagValue(Token::StartOfLine , AtStartOfLine);
     Tok.setFlagValue(Token::LeadingSpace, HasLeadingSpace || NextTokGetsSpace);
     if (CurToken == 0)
       Tok.setFlag(Token::LeadingEmptyMacro);
+    // If we ended up being empty without a token to attach CommaElision to,
+    // we set the token being created as potentially needing a comma elision.
+    if (UnfulfilledCommaElision) {
+      Tok.setFlag(Token::CommaBeforeNeedsElision);
+      UnfulfilledCommaElision = false;
+    }
     return PP.HandleEndOfTokenLexer(Tok);
   }
 
   SourceManager &SM = PP.getSourceManager();
 
   // If this is the first token of the expanded result, we inherit spacing
   // properties later.
   bool isFirstToken = CurToken == 0;
 
   // Get the next token to return.
+  bool CommaBeforeNeedsElision = Tok.getFlag(Token::CommaBeforeNeedsElision);
+  bool CommaBeforeElided = Tok.getFlag(Token::CommaBeforeElided);
   Tok = Tokens[CurToken++];
+  // Propogate the comma elision to the next token if we haven't already.
+  // This only happens in teh case where the previous token wasn't emitted,
+  // in this call to PP::Lex.
+  if (CommaBeforeNeedsElision)
+    Tok.setFlag(Token::CommaBeforeNeedsElision);
+  if (CommaBeforeElided)
+    Tok.setFlag(Token::CommaBeforeElided);
 
   bool TokenIsFromPaste = false;
 
   // If this token is followed by a token paste (##) operator, paste the tokens!
   // Note that ## is a normal token when not expanding a macro.
   if (!isAtEnd() && Macro &&
       (Tokens[CurToken].is(tok::hashhash) ||
        // Special processing of L#x macros in -fms-compatibility mode.
if (!Tok.isAnnotation() && Tok.getIdentifierInfo() != nullptr) {
     if (II->isPoisoned() && TokenIsFromPaste) {
       PP.HandlePoisonedIdentifier(Tok);
     }
 
     if (!DisableMacroExpansion && II->isHandleIdentifierCase())
       return PP.HandleIdentifier(Tok);
   }
 
+  // If the current token is a comma, it might be subject to MSVC's comma
+  // elision, so look ahead and see if this is the case. Don't do this in
+  // LookAhead mode, since consecutive commas need to result in only a single
+  // elision.
+  if (Tok.is(tok::comma) && PP.getLangOpts().MSVCCompat) {
+    const Token &NextTok = PP.LookAhead(0);
+    // If we need to ignore this comma don't return successfully, and allow the
+    // next token to get picked up in lexing.
+    if (NextTok.getFlag(Token::CommaBeforeNeedsElision) &&
+        !NextTok.getFlag(Token::CommaBeforeElided)) {
+      Tok.setFlag(Token::CommaBeforeElided);
+      return false;
+    }
+  }
+
   // Otherwise, return a normal token.
   return true;
 }
 
 /// PasteTokens - Tok is the LHS of a ## operator, and CurToken is the ##
 /// operator.  Read the ## and RHS, and paste the LHS/RHS together.  If there
 /// are more ## after it, chomp them iteratively.  Return the result as Tok.
 /// If this returns true, the caller should immediately return the token.

test/Preprocessor/microsoft-ext.c

 TWELVE_EMPTY_A: M(1,TWO)
 // CHECK: TWELVE_EMPTY_A: 1
 TWENTYONE_EMPTY:M(TWO, 1)
 // CHECK: TWENTYONE_EMPTY:1
 TWENTYONE_EMPTY_A: M(TWO, 1)
 // CHECK: TWENTYONE_EMPTY_A: 1
 #undef TWO
 #undef M
+
+// Comma suppressed before an empty variadic macro expansion.
+#define EMPTY
+#define PRINT(s, ...) printf(s, __VA_ARGS__)
+PRINT("error", EMPTY);
+// CHECK: printf("error");
+#undef PRINT
+#undef EMPTY
+
+// Comma suppressed before an empty variadic macro expansion, even through
+// function indirections.  
+#define M(...) __VA_ARGS__
+#define X(...) __VA_ARGS__,b;
+#define Y(x,...) X(a, M(__VA_ARGS__))
+#define Z(x,...) a,X(M(__VA_ARGS__))
+#define Q(x,...) a,M(__VA_ARGS__);
+int Y(xyz)
+// CHECK: int a,b;
+int Z(xyz)
+// CHECK: int a,b;
+int Q(xyz)
+// CHECK: int a;
+int d,X()
+// CHECK: int d,,b;
+#undef Q
+#undef Z
+#undef Y
+#undef X
+#undef M
+