[C++2a][Preprocessor] Implement p0306 __VA_OPT__ (Comma omission and comma deletion)

[faisalv]

Summary

This patch implements an extension to the preprocessor: VA_OPT which expands into its contents if variadic arguments are supplied, or behaves as an empty token if none.

• currently it is only enabled for C++2a (we could always enable this for various other dialects with the appropriate extension or compatibility warnings)

Given:

#define F(a,...) a #__VA_OPT__(a ## a)  a ## __VA_OPT__(__VA_ARGS__)

A few technicalities (most which were clarified through private correspondence between rsmith, hubert and thomas) are worth mentioning:

• the call F(,) Does not supply any tokens for the variadic arguments and hence VA_OPT behaves as a placeholder
• when expanding VA_OPT (for e.g. F(,1) token pasting occurs eagerly within its contents
• a hash or a hashhash prior to VA_OPT does not inhibit expansion of arguments if they are the first token within VA_OPT.
• essentially, when a variadic argument is supplied, argument substitution occurs within the contents as does stringification and concatenation - and this is substituted, potentially strinigified and concatenated content (but not rescanned) is inserted into the macro expansions token stream just prior to the entire stream being rescanned and concatenated.

See wg21.link/P0306 for further details.
See the example files for more involved examples.

Would greatly appreciate timely feedback =)

[rsmith]

I think we need more consideration of what the end result of __VA_OPT__ should be, and particularly how it should affect the SourceLocation data. It seems there are two models:

1. Model __VA_OPT__ following the specification in the standard: form an imaginary macro argument by expanding the contents, and then substitute that for the __VA_OPT__(...) tokens
2. Model __VA_OPT__( ... ) as if it is just some tokens "in the way" of regular expansion, with macro expansion just walking into the contents.

In particular, option 1 would give us a macro argument expansion sloc for each __VA_OPT__ where option 2 would not. I think it's somewhat unclear which is the better model, but option 1 is more faithful to the standard wording at least.

If we want to try option 1, I think the way to model it would be to snip the __VA_OPT__(...) out of the replacement list when forming the MacroInfo, to store separate replacement lists for each such sequence, and to build additional arguments in the MacroArgs for each such replacement when we expand a macro using __VA_OPT__.

Have you considered the other option? If so, why do you think this way is better? (I haven't formed an opinion myself yet, but my default inclination would be to follow the model in the standard rather than using a different one, even one that is formally equivalent.)

include/clang/Basic/DiagnosticLexKinds.td

351

Seems weird to mention C99 in a diagnostic about a C++20 feature. Maybe just drop the "C99" here?

355

Including the "( contents" here seems pretty awkward. Can we just use the normal style of a "missing ) for this (" diagnostic, with a note pointing to the '('?

360

"at start within VA_OPT" might be clearer as "at start of VA_OPT argument" or similar.

include/clang/Lex/Preprocessor.h

133

Drop the "(contents)" from the comment here. The identifier we're tracking here is just __VA_OPT__.

include/clang/Lex/VariadicMacroSupport.h

1

Typo "gaurds".

24–27

Please factor out this class (for the __VA_ARGS__ case) and commit it separately.

30–31

Don't look these up by string each time, instead grab the value cached on the preprocessor.

35

Rather than repeating this connection between language mode and this particular feature here, maybe check whether the cached IdentifierInfo* on the preprocessor is null?

57–59

LLVM / Clang style uses CamelCase for names such as this.

61

This should not be called State if what it's tracking is the number of nested parens. Please give this a better name.

74

Doc comments should start with three slashes, not repeat the function name, and be formatted as a sentence (leading capital letter, terminating period).

lib/Lex/TokenLexer.cpp

192

It's a bit weird to use the unexpanded argument here:

#define EMPTY
#define X(a, ...) __VA_OPT__(not) empty
X(1) // empty
X(1, ) // empty
X(1, EMPTY) // not empty

... but I think this ultimately is the right thing. Especially when you consider that #__VA_ARGS__ would produce "" for the first two cases and "EMPTY" for the third. Maybe extend your comment with the empty macro case?

[faisalv]

Hi Richard,

This patch attempts to incorporate all the changes you requested.

You asked for some consideration on the rationale for sticking with my approach (as opposed to the alternative you posed), so here are my reasons (FWTW) for not modeling the VA_OPT as a macro argument:

• the standard does not specify it as behaving exactly as a macro argument (specifically no rescanning, after placemarker token removal) - and so i would have to special case the argument expansion logic - which currently composes well by just launching a tokenlexer on the argument.
• it would be the only argument that would need to have *other* arguments substituted into it
• unlike every other argument it would not be represented by just 1 token
• in regards to the source-location mapping, my sense is that the ExpansionInfo that represents the various expansions might need to add a representation for this 'fourth' kind of expansion (since expanded args are represented by a valid LocStart but an invalid LocEnd, and since this is more than one token, things could get hairy)
• it might require complicating 'Parsing' the macro definition (and the MacroArgs, MacroInfo objects) and the general tokenLexer, ExpandFunctionArguments() logic more than any gained conceptual elegance (in my opinion)

Thanks Richard - Look forward to your review and thoughts!

[faisalv]

*ping*

[majnemer]

include/clang/Lex/TokenLexer.h

169

I think llvm::ArrayRef<Token>() can just be llvm::None.

[rsmith]

This generally looks good. I have some specific ideas about moving more of the recognition of the __VA_OPT__ ( ... ) pattern into VAOptDefinitionContext, so if you prefer you can go ahead with something like this patch and I'll do the refactoring afterwards.

include/clang/Basic/DiagnosticLexKinds.td

360

'##' cannot appear at end of __VA_OPT__ argument please, to match the change to the previous diagnostic.

include/clang/Lex/VariadicMacroSupport.h

1

nit: "state machines" and "scope guards" should not be hyphenated

69

Please add a documentation comment explaining what this is. Maybe something like:

"A class for tracking whether we're inside a VA_OPT during a traversal of the tokens of a variadic macro."

119

And here:

"A class for tracking whether we're inside a VA_OPT during traversal of the tokens of a macro as part of macro expansion."

lib/Lex/PPDirectives.cpp

2384–2418

I think I would be inclined to move more of this logic into VAOptDefinitionContext, maybe a TokenState process(const Token&) member that returns an enum saying whether the token should be handled normally, skipped, diagnosed due to missing lparen, diagnosed due to nested va_opt, ...

lib/Lex/Preprocessor.cpp

130

Have you considered allowing this as an extension in other language modes?

lib/Lex/TokenLexer.cpp

189–195

Please make this a member function of MacroArgs (passing in the MacroInfo*).

275–284

Is this extra check really necessary? We only set PasteBefore to true if there was a hashhash token in the token stream prior to the __VA_OPT__, so it seems like we should be able to assume the token is still there now.

285

I think it might be more obvious to move this hasStringifyOrCharifyBefore check upwards, and define a bool IsPlacemarkerToken = !NumVAOptTokens && !VCtx.hasStringifyOrCharifyBefore();, then conditionalize these special cases on IsPlacemarkerToken not !NumVAOptTokens.

321–322

ConcatenatedVAOPTResultToks.back() = LHS;

357

The first part of this while condition has no effect. The second part has an effect, but it's tremendously confusing to put this check here, when it corresponds only to the continue on line 251. Can you move the I != E check to line 251 and make this just be a while (true) loop?

(I also think that having the normal fallthrough path through the loop be an exit path is far from ideal. Moving to a process(Token) mechanism on the VAOpt...Context class should help with this.)

418–420

This seems excessive. Just "unexpected ## in result tokens" would do, I think.

572–575

This is a fun corner case. :)

723–725

Please go ahead and do that refactoring now :)

[faisalv]

committed as r315840.
https://reviews.llvm.org/rL315840

include/clang/Basic/DiagnosticLexKinds.td

 def ext_pp_comma_expr : Extension<"comma operator in operand of #if">;
 def ext_pp_bad_vaargs_use : Extension<
   "__VA_ARGS__ can only appear in the expansion of a C99 variadic macro">;
+
+def ext_pp_bad_vaopt_use : Extension<
+  "__VA_OPT__ can only appear in the expansion of a C99 variadic macro">;
+def err_pp_missing_lparen_in_vaopt_use : Error<
+  "missing '(' following __VA_OPT__">;
+def err_pp_missing_rparen_in_vaopt_use : Error<
+  "missing ')' following __VA_OPT__( contents ">;
+def err_pp_vaopt_nested_use : Error<
+  "__VA_OPT__ cannot be nested within its own replacement tokens">;
+
+def err_vaopt_paste_at_start : Error<
+  "'##' cannot appear at start within __VA_OPT__">;
+
+def err_vaopt_paste_at_end : Error<"'##' cannot appear at end within __VA_OPT__">;
+
+
 def ext_pp_macro_redef : ExtWarn<"%0 macro redefined">, InGroup<MacroRedefined>;
 def ext_variadic_macro : Extension<"variadic macros are a C99 feature">,
   InGroup<VariadicMacros>;

include/clang/Lex/Preprocessor.h

   IdentifierInfo *Ident_Pragma, *Ident__pragma;    // _Pragma, __pragma
   IdentifierInfo *Ident__identifier;               // __identifier
   IdentifierInfo *Ident__VA_ARGS__;                // __VA_ARGS__
+  IdentifierInfo *Ident__VA_OPT__;                 // __VA_OPT__(contents)
   IdentifierInfo *Ident__has_feature;              // __has_feature
   IdentifierInfo *Ident__has_extension;            // __has_extension
   IdentifierInfo *Ident__has_builtin;              // __has_builtin

include/clang/Lex/TokenLexer.h

 #define LLVM_CLANG_LEX_TOKENLEXER_H
 
 #include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/ArrayRef.h"
 
 namespace clang {
   class MacroInfo;

   /// are is another ## after it, chomp it iteratively.  Return the result as
   /// Tok.  If this returns true, the caller should immediately return the
   /// token.
-  bool PasteTokens(Token &Tok);
+  bool PasteTokens(Token &Tok,
+                   llvm::ArrayRef<Token> AltTokens = llvm::ArrayRef<Token>(),
+                   unsigned int *const AltCurTokenIdx = nullptr);
 
   /// Expand the arguments of a function-like macro so that we can quickly
   /// return preexpanded tokens from Tokens.

include/clang/Lex/VariadicMacroSupport.h

+//===- VariadicMacroSupport.h - state-machines and scope-gaurds -*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines support types to help with preprocessing variadic macro 
+// (i.e. macros that use: ellipses __VA_ARGS__ __VA_OPT__ ) definitions and 
+// expansions.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_LEX_VARIADICMACROSUPPORT_H
+#define LLVM_CLANG_LEX_VARIADICMACROSUPPORT_H
+
+#include "clang/Lex/Preprocessor.h"
+
+namespace clang {
+  class Preprocessor;
+
+  class VariadicMacroScopeGuard {
+    const Preprocessor &PP;
+    IdentifierInfo &Ident__VA_ARGS__;
+    IdentifierInfo &Ident__VA_OPT__;
+  public:
+    VariadicMacroScopeGuard(const Preprocessor &P)
+        : PP(P), Ident__VA_ARGS__(*PP.getIdentifierInfo("__VA_ARGS__")),
+          Ident__VA_OPT__(*PP.getIdentifierInfo("__VA_OPT__")) {
+      // These identifiers are only allowed within a variadic macro...
+      assert(Ident__VA_ARGS__.isPoisoned() &&
+             "__VA_ARGS__ should be poisoned!");
+      assert(!PP.getLangOpts().CPlusPlus2a ||
+             (Ident__VA_OPT__.isPoisoned() &&
+              "__VA_OPT__ should be poisoned!"));
+    }
+    void enterScope() {
+      Ident__VA_ARGS__.setIsPoisoned(false);
+      if (PP.getLangOpts().CPlusPlus2a)
+        Ident__VA_OPT__.setIsPoisoned(false);
+    }
+    void exitScope() {
+      // Make sure these variadic macros are disabled once we are done handing
+      // the macro definition.
+      Ident__VA_ARGS__.setIsPoisoned(true);
+      if (PP.getLangOpts().CPlusPlus2a)
+        Ident__VA_OPT__.setIsPoisoned(true);
+    }
+    ~VariadicMacroScopeGuard() { exitScope(); }
+  };
+
+  class VAOptDefinitionContext {
+    Preprocessor &PP;
+    enum VAOptState {
+      OUTSIDE_VAOPT = -1,
+      MATCHED_CLOSING_PARENS_OF_VAOPT = 0,
+      MATCHED_OPENING_PARENS_OF_VAOPT = 1
+    };
+    int State = OUTSIDE_VAOPT;
+    const IdentifierInfo *const Ident__VA_OPT__;
+  public:
+    VAOptDefinitionContext(Preprocessor &PP)
+        : PP(PP), Ident__VA_OPT__(PP.getIdentifierInfo("__VA_OPT__")) {}
+
+    bool isVAOptToken(const Token &T) const {
+      return T.getIdentifierInfo() == Ident__VA_OPT__ &&
+             PP.getLangOpts().CPlusPlus2a;
+    }
+
+    bool hasEntered() const { return State != OUTSIDE_VAOPT; }
+    
+    // enter - call this as soon as you see __VA_OPT__ and '('
+    void enter() {
+      assert(!hasEntered() && "Must NOT be within VAOPT context to call this");
+      State = MATCHED_OPENING_PARENS_OF_VAOPT;
+    }
+    
+    // exit - call this once you see the non-nested closing ')' parens.
+    void exit() {
+      assert(hasEntered() && "Must be within VAOPT context to call this");
+      State = OUTSIDE_VAOPT;
+    }
+
+    // Returns true if this was the (non-nested) closing paren for VAOPT.
+    bool sawRParen() {
+      assert(hasEntered() && "Must be within VAOPT context to call this");
+      return !--State;
+    }
+    
+    void sawLParen() {
+      assert(hasEntered() && "Must be within VAOPT context to call this");
+      ++State;
+    }
+    
+  };
+
+  class VAOptExpansionContext : VAOptDefinitionContext {
+    // IndexOfFirstVAOptTokenWithinSubstitutedTokens : when != -1, contains the
+    // index of the first token within vaopt (so we know where to start eager
+    // token-pasting and stringification) - within the substituted tokens of the
+    // function-like macro's new replacement list.
+    int IndexOfFirstVAOptTokenWithinSubstitutedTokens = -1;
+    bool LeadingSpaceForStringifiedToken = false;
+    
+    Token SyntheticEOFToken;
+    bool PasteBefore = false;
+    const Token *VAOptToken = nullptr;
+    const Token *PrecedingStringifyToken = nullptr;
+  public:
+    VAOptExpansionContext(Preprocessor &PP) : VAOptDefinitionContext(PP) {
+      SyntheticEOFToken.startToken();
+      SyntheticEOFToken.setKind(tok::eof);
+    }
+    
+    const Token &getEOFTok() const { return SyntheticEOFToken; }
+
+    void markEntireContentsForStringification(const bool HasLeadingSpace,
+                                              const Token &StringifyTok) {
+      assert((StringifyTok.is(tok::hash) || StringifyTok.is(tok::hashat)) &&
+             "Stringify token must be '#' or '#@'");
+      PrecedingStringifyToken = &StringifyTok;
+      LeadingSpaceForStringifiedToken = HasLeadingSpace;
+    }
+
+    void markPasteBefore() {
+      assert(hasEntered() &&
+             "Must only be called if we have entered VAOPT context");
+      PasteBefore = true;
+    }
+
+    bool hasPasteBefore() const {
+      assert(hasEntered() &&
+             "Must only be called if we have entered VAOPT context");
+      return PasteBefore;
+    }
+
+    bool stringify() const {
+      assert(hasEntered() &&
+             "Must only be called if we have entered VAOPT context");
+      return PrecedingStringifyToken;
+    }
+    const Token &getStringifyToken() const {
+      assert(stringify() && "Must be marked for stringification");
+      return *PrecedingStringifyToken;
+    }
+
+    void setIndexOfFirstToken(unsigned int Idx) {
+      assert(hasEntered() &&
+             "Must only be called if we have entered VAOPT context");
+      IndexOfFirstVAOptTokenWithinSubstitutedTokens = Idx;
+    }
+    
+    unsigned int getIndexOfFirstToken() const {
+      assert(hasEntered() &&
+             "Must only be called if we have entered VAOPT context");
+      return IndexOfFirstVAOptTokenWithinSubstitutedTokens;
+    }
+    
+    bool getLeadingSpaceForStringifiedToken() const {
+      assert(stringify() &&
+             "Must only be called if this has been marked for stringification");
+      return LeadingSpaceForStringifiedToken;
+    }
+    
+    void exit() {
+      VAOptDefinitionContext::exit();
+      IndexOfFirstVAOptTokenWithinSubstitutedTokens = -1;
+ 
+      LeadingSpaceForStringifiedToken = false;
+      PasteBefore                     = false;
+      VAOptToken                      = nullptr;
+      PrecedingStringifyToken         = nullptr;
+    }
+    
+    void enter(const Token &VAOTok) {
+      assert(isVAOptToken(VAOTok) && "Must enter with a valid __VAOPT__ token");
+      VAOptDefinitionContext::enter();
+      VAOptToken = &VAOTok;
+    }
+    
+    const Token &getVAOptToken() const {
+      assert(hasEntered() && "Must have entered context before calling this");
+      return *VAOptToken;
+    }
+    
+    using VAOptDefinitionContext::isVAOptToken;
+    using VAOptDefinitionContext::hasEntered;
+    
+
+    using VAOptDefinitionContext::sawRParen;
+    using VAOptDefinitionContext::sawLParen;
+  };
+}  // end namespace clang
+
+#endif

lib/Lex/PPDirectives.cpp

 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Lex/PTHLexer.h"
 #include "clang/Lex/Token.h"
+#include "clang/Lex/VariadicMacroSupport.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"

   Token Tok;
   LexUnexpandedToken(Tok);
 
+  VariadicMacroScopeGuard VariadicMacroScopeGuard(*this);
+
   // If this is a function-like macro definition, parse the argument list,
   // marking each of the identifiers as being used as macro arguments.  Also,
   // check other constraints on the first token of the macro body.

     }
 
     // If this is a definition of a variadic C99 function-like macro, not using
-    // the GNU named varargs extension, enabled __VA_ARGS__.
+    // the GNU named varargs extension, enable __VA_ARGS__.
 
-    // "Poison" __VA_ARGS__, which can only appear in the expansion of a macro.
-    // This gets unpoisoned where it is allowed.
-    assert(Ident__VA_ARGS__->isPoisoned() && "__VA_ARGS__ should be poisoned!");
-    if (MI->isC99Varargs())
-      Ident__VA_ARGS__->setIsPoisoned(false);
+    if (MI->isC99Varargs()) {
+      VariadicMacroScopeGuard.enterScope();
+    }
 
     // Read the first token after the arg list for down below.
     LexUnexpandedToken(Tok);

     // Otherwise, read the body of a function-like macro.  While we are at it,
     // check C99 6.10.3.2p1: ensure that # operators are followed by macro
     // parameters in function-like macro expansions.
+
+    VAOptDefinitionContext VAOCtx(*this);
+
     while (Tok.isNot(tok::eod)) {
       LastTok = Tok;
       
       if (!Tok.isOneOf(tok::hash, tok::hashat, tok::hashhash)) {
         MI->AddTokenToBody(Tok);
 
+        if (VAOCtx.isVAOptToken(Tok)) {
+          // If we're already within a VAOPT, emit an error.
+          if (VAOCtx.hasEntered()) {
+            Diag(Tok, diag::err_pp_vaopt_nested_use);
+            return nullptr;
+          }
+          // Ensure VAOPT is followed by a '(' .
+          LexUnexpandedToken(Tok);
+          if (Tok.isNot(tok::l_paren)) {
+            Diag(Tok, diag::err_pp_missing_lparen_in_vaopt_use);
+            return nullptr;
+          }
+          MI->AddTokenToBody(Tok);
+          VAOCtx.enter();
+          LexUnexpandedToken(Tok);
+          if (Tok.is(tok::hashhash)) {
+            Diag(Tok, diag::err_vaopt_paste_at_start);
+            return nullptr;
+          }
+          continue;
+        } else if (VAOCtx.hasEntered()) {
+          if (Tok.is(tok::r_paren)) {
+            if (VAOCtx.sawRParen()) {
+              const unsigned NumTokens = MI->getNumTokens();
+              assert(NumTokens >= 3 && "Must have seen at least __VA_OPT__( "
+                                       "and a subsequent tok::r_paren");
+              if (MI->getReplacementToken(NumTokens - 2).is(tok::hashhash)) {
+                Diag(Tok, diag::err_vaopt_paste_at_end);
+                return nullptr;
+              }
+              VAOCtx.exit();
+            }
+          } else if (Tok.is(tok::l_paren)) {
+            VAOCtx.sawLParen();
+          }
+        }
         // Get the next token of the macro.
         LexUnexpandedToken(Tok);
         continue;

         continue;
       }
 
+      // Our Token is a stringization operator.
       // Get the next token of the macro.
       LexUnexpandedToken(Tok);
 
-      // Check for a valid macro arg identifier.
-      if (Tok.getIdentifierInfo() == nullptr ||
-          MI->getParameterNum(Tok.getIdentifierInfo()) == -1) {
+      // Check for a valid macro arg identifier or __VA_OPT__.
+      if (!VAOCtx.isVAOptToken(Tok) &&
+          (Tok.getIdentifierInfo() == nullptr ||
+           MI->getParameterNum(Tok.getIdentifierInfo()) == -1)) {
 
         // If this is assembler-with-cpp mode, we accept random gibberish after
         // the '#' because '#' is often a comment character.  However, change

         } else {
           Diag(Tok, diag::err_pp_stringize_not_parameter)
             << LastTok.is(tok::hashat);
-
-          // Disable __VA_ARGS__ again.
-          Ident__VA_ARGS__->setIsPoisoned(true);
           return nullptr;
         }
       }

 
       // 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);
+      // If the token following '#' is VAOPT, let the next iteration handle it
+      // and check it for correctness, otherwise add the token and prime the
+      // loop with the next one.
+      if (!VAOCtx.isVAOptToken(Tok)) {
+        MI->AddTokenToBody(Tok);
+        LastTok = Tok;
+
+        // Get the next token of the macro.
+        LexUnexpandedToken(Tok);
+      }
     }
+    if (VAOCtx.hasEntered()) {
+      Diag(Tok, diag::err_pp_missing_rparen_in_vaopt_use);
+      return nullptr;
+    }
   }
+  
   MI->setDefinitionEndLoc(LastTok.getLocation());
-  // Disable __VA_ARGS__ again.
-  Ident__VA_ARGS__->setIsPoisoned(true);
-
   return MI;
 }
 /// HandleDefineDirective - Implements \#define.  This consumes the entire macro

lib/Lex/Preprocessor.cpp

 
   // We haven't read anything from the external source.
   ReadMacrosFromExternalSource = false;
-  
-  // "Poison" __VA_ARGS__, which can only appear in the expansion of a macro.
-  // This gets unpoisoned where it is allowed.
+  // "Poison" __VA_ARGS__, __VA_OPT__ which can only appear in the expansion of
+  // a macro. They get unpoisoned where it is allowed.
   (Ident__VA_ARGS__ = getIdentifierInfo("__VA_ARGS__"))->setIsPoisoned();
   SetPoisonReason(Ident__VA_ARGS__,diag::ext_pp_bad_vaargs_use);
-  
+  if (getLangOpts().CPlusPlus2a) {
+    (Ident__VA_OPT__ = getIdentifierInfo("__VA_OPT__"))->setIsPoisoned();
+    SetPoisonReason(Ident__VA_OPT__,diag::ext_pp_bad_vaopt_use);
+  }
+
   // Initialize the pragma handlers.
   RegisterBuiltinPragmas();
   

   // unpoisoned it if we're defining a C99 macro.
   if (II.isOutOfDate()) {
     bool CurrentIsPoisoned = false;
-    if (&II == Ident__VA_ARGS__)
-      CurrentIsPoisoned = Ident__VA_ARGS__->isPoisoned();
+    const bool IsSpecialVariadicMacro =
+        &II == Ident__VA_ARGS__ || &II == Ident__VA_OPT__;
+    if (IsSpecialVariadicMacro)
+      CurrentIsPoisoned = II.isPoisoned();
 
     updateOutOfDateIdentifier(II);
     Identifier.setKind(II.getTokenID());
 
-    if (&II == Ident__VA_ARGS__)
+    if (IsSpecialVariadicMacro)
       II.setIsPoisoned(CurrentIsPoisoned);
   }
   

lib/Lex/TokenLexer.cpp

 #include "clang/Lex/MacroArgs.h"
 #include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/VariadicMacroSupport.h"
 #include "llvm/ADT/SmallString.h"
 
 using namespace clang;

   // we install the newly expanded sequence as the new 'Tokens' list.
   bool MadeChange = false;
 
+  // Check to see if this invocation supplied any variadic arguments. Use
+  // this information to expand __VA_OPT__  into its arguments (if true) or
+  // empty (if false) appropriately.  Note, for a macro defined as: 
+  // 
+  //    #define F(a, ...) __VA_OPT__(x x x)
+  //    F(,) <-- This is NOT called with variadic arguments (tokens need to be
+  //             supplied).
+  const bool CalledWithVariadicArguments = [this] {
+    if (Macro->isVariadic()) {
+      const int VariadicArgIndex = Macro->getNumParams() - 1;
+      const Token *VarArgs = ActualArgs->getUnexpArgument(VariadicArgIndex);
+      return VarArgs->isNot(tok::eof);
+    }
+    return false;
+  }();
+  
+  VAOptExpansionContext VCtx(PP);
+  
   for (unsigned I = 0, E = NumTokens; I != E; ++I) {
-    // If we found the stringify operator, get the argument stringified.  The
-    // preprocessor already verified that the following token is a macro name
-    // when the #define was parsed.
+    
     const Token &CurTok = Tokens[I];
+    
     // We don't want a space for the next token after a paste
     // operator.  In valid code, the token will get smooshed onto the
     // preceding one anyway. In assembler-with-cpp mode, invalid

     if (I != 0 && !Tokens[I-1].is(tok::hashhash) && CurTok.hasLeadingSpace())
       NextTokGetsSpace = true;
 
+    if (VCtx.isVAOptToken(CurTok)) {
+      MadeChange = true;
+      assert(Tokens[I + 1].is(tok::l_paren) &&
+             "__VA_OPT__ must be followed by '('");
+      
+      const bool PasteBefore = I ? Tokens[I - 1].is(tok::hashhash) : false;
+      ++I; // Skip the l_paren
+      VCtx.enter(CurTok);
+      if (PasteBefore)
+        VCtx.markPasteBefore();
+      VCtx.setIndexOfFirstToken(ResultToks.size());
+      continue;
+    }
+
+    // We have entered into the __VA_OPT__ context, so handle tokens
+    // appropriately.
+    if (VCtx.hasEntered()) {
+      // If we are about to process a token that is an argument to __VA_OPT__ we
+      // need to:
+      //  1) if macro was called without variadic arguments skip all the
+      //  intervening tokens
+      //  2) always process the closing right-parens, and skip the current 
+      //  iteration.
+      //  3) otherwise let the current iteration process this token.
+
+      // Use this variable to skip to the next iteration as described above.
+      bool SkipToNextIterationOfOuterLoop = !CalledWithVariadicArguments;
+      do {
+        if (Tokens[I].is(tok::l_paren))
+          VCtx.sawLParen();
+
+        // Continue skipping tokens within __VA_OPT__ if the macro was not
+        // called with variadic arguments, else let the rest of the loop handle
+        // this token. Note sawRParen() returns true only if the r_paren matches
+        // the closing r_paren of the __VA_OPT__.
+        if (!Tokens[I].is(tok::r_paren) || !VCtx.sawRParen()) {
+          if (!CalledWithVariadicArguments) {
+            ++I; // Skip Tokens in between.
+            continue /*inner do loop*/;
+          }
+          // The macro was called with variadic arguments, and we do not have a
+          // closing rparen so let the outer loop process this token.
+          break;
+        }
+
+        // Current token is the closing r_paren which marks the end of the
+        // __VA_OPT__ invocation, so handle any place-marker pasting (if
+        // empty) by removing hashhash either before (if exists) or after. And
+        // also stringify the entire contents if VAOPT was preceded by a hash,
+        // but do so only after if any token concatenation needs to occur within
+        // the contents of VAOPT.
+        const int ResultToksStartIdx = VCtx.getIndexOfFirstToken();
+        const unsigned int NumVAOptTokens =
+            ResultToks.size() - ResultToksStartIdx;
+        const bool HasPasteAfter =
+            (I + 1 != E) ? Tokens[I + 1].is(tok::hashhash) : false;
+        // We'll have to eat any hashhashes before or after these empty tokens.
+        if (!NumVAOptTokens) {
+          if (VCtx.hasPasteBefore()) {
+            assert(!VCtx.stringify() &&
+                   "Can't have both hashhash and hash just before __VA_OPT__");
+
+            // Either the substituted/new tokens have a hashhash before
+            // __VA_OPT__ or placeholder-tokens have led to the previous
+            // hashhash being skipped (we couldn't have added any tokens after
+            // __VAOPT__ ).
+            if (ResultToks.size() && ResultToks.back().is(tok::hashhash))
+              ResultToks.pop_back();
+          } else if (HasPasteAfter && !VCtx.stringify()) {
+            ++I; // Skip the hashhash.
+          }
+        }
+
+        if (VCtx.stringify()) {
+          // Replace all the tokens just added from within VAOPT into a single
+          // stringified token. This requires token-pasting to eagerly occur
+          // within these tokens. If either the contents of VAOPT were empty
+          // or the macro wasn't called with any variadic arguments result in
+          // an empty string.
+
+          Token *const VAOPTTokens =
+              NumVAOptTokens ? &ResultToks[ResultToksStartIdx] : nullptr;
+
+          SmallVector<Token, 64> ConcatenatedVAOPTResultToks;
+          // FIXME: Should we keep track within VCtx that we did or didnot
+          // encounter pasting - and only then perform this loop.
+
+          for (unsigned int CurTokenIdx = 0; CurTokenIdx != NumVAOptTokens;
+               ++CurTokenIdx) {
+            const unsigned int PrevTokenIdx = CurTokenIdx;
+
+            if (VAOPTTokens[CurTokenIdx].is(tok::hashhash)) {
+              assert(CurTokenIdx != 0 &&
+                     "Can not have __VAOPT__ contents begin with a ##");
+              Token &LHS = VAOPTTokens[CurTokenIdx - 1];
+              PasteTokens(LHS, llvm::makeArrayRef(VAOPTTokens, NumVAOptTokens),
+                          &CurTokenIdx);
+              // CurTokenIdx is either the same as NumTokens or one past the
+              // last token concatenated.
+              // PrevTokenIdx is the index of the hashhash
+              const unsigned NumTokensPastedTogether =
+                  CurTokenIdx - PrevTokenIdx + 1;
+              // Replace the token prior to the first ## in this iteration.
+              ConcatenatedVAOPTResultToks[ConcatenatedVAOPTResultToks.size() -
+                                          1] = LHS;
+              if (CurTokenIdx == NumVAOptTokens)
+                break;
+              ConcatenatedVAOPTResultToks.push_back(VAOPTTokens[CurTokenIdx]);
+            } else {
+              ConcatenatedVAOPTResultToks.push_back(VAOPTTokens[CurTokenIdx]);
+            }
+          }
+
+          ConcatenatedVAOPTResultToks.push_back(VCtx.getEOFTok());
+          // Get the SourceLocation that represents the start location within
+          // the macro definition that marks where this string is substituted
+          // into: i.e. the __VA_OPT__ and the ')' within the spelling of the
+          // macro definition.
+          const SourceLocation ExpansionLocStartWithinMacro =
+              getExpansionLocForMacroDefLoc(VCtx.getVAOptToken().getLocation());
+          const SourceLocation ExpansionLocEndWithinMacro =
+              getExpansionLocForMacroDefLoc(Tokens[I].getLocation());
+
+          Token StringifiedVAOPT = MacroArgs::StringifyArgument(
+              &ConcatenatedVAOPTResultToks[0], PP,
+              VCtx.getStringifyToken().is(tok::hashat) /*Charify*/,
+              ExpansionLocStartWithinMacro, ExpansionLocEndWithinMacro);
+
+          if (VCtx.getLeadingSpaceForStringifiedToken())
+            StringifiedVAOPT.setFlag(Token::LeadingSpace);
+
+          StringifiedVAOPT.setFlag(Token::StringifiedInMacro);
+
+          ResultToks.resize(ResultToksStartIdx + 1);
+          ResultToks[ResultToksStartIdx] = StringifiedVAOPT;
+        }
+        VCtx.exit();
+        SkipToNextIterationOfOuterLoop = true;
+        break /*out of inner loop*/;
+      } while (!CalledWithVariadicArguments && I != E);  
+      
+      if (I == E) break /*out of outer loop*/;
+      
+      if (SkipToNextIterationOfOuterLoop)
+        continue;
+    }
+    // If we found the stringify operator, get the argument stringified.  The
+    // preprocessor already verified that the following token is a macro 
+    // parameter or __VA_OPT__ when the #define was lexed.
+    
     if (CurTok.isOneOf(tok::hash, tok::hashat)) {
       int ArgNo = Macro->getParameterNum(Tokens[I+1].getIdentifierInfo());
-      assert(ArgNo != -1 && "Token following # is not an argument?");
-
+      assert((ArgNo != -1 || VCtx.isVAOptToken(Tokens[I + 1])) &&
+             "Token following # is not an argument or __VA_OPT__!");
+      
+      if (ArgNo == -1) {
+        // Handle the __VA_OPT__ case.
+        VCtx.markEntireContentsForStringification(NextTokGetsSpace, CurTok);
+        continue;
+      }
+      // Else handle the simple argument case.
       SourceLocation ExpansionLocStart =
           getExpansionLocForMacroDefLoc(CurTok.getLocation());
       SourceLocation ExpansionLocEnd =

 
       ResultToks.push_back(Res);
       MadeChange = true;
-      ++I;  // Skip arg name.
+      ++I;  // Skip param name.
       NextTokGetsSpace = false;
       continue;
     }

       !ResultToks.empty() && ResultToks.back().is(tok::hashhash);
     bool PasteBefore = I != 0 && Tokens[I-1].is(tok::hashhash);
     bool PasteAfter = I+1 != E && Tokens[I+1].is(tok::hashhash);
-    assert(!NonEmptyPasteBefore || PasteBefore);
 
+    assert(
+        !NonEmptyPasteBefore || PasteBefore ||
+        (I > 3 && VCtx.isVAOptToken(Tokens[I - 2]) &&
+         Tokens[I - 3].is(tok::hashhash)) &&
+            "If the substituted ResultToks has hashhash as its most recent "
+            "token, but the original replacement tokens doesn't, then this can "
+            "only happen if we just entered into VAOPT after hashhash");
+
     // Otherwise, if this is not an argument token, just add the token to the
     // output buffer.
     IdentifierInfo *II = CurTok.getIdentifierInfo();

   Tok = Tokens[CurToken++];
 
   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 &&

   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.
-bool TokenLexer::PasteTokens(Token &Tok) {
+bool TokenLexer::PasteTokens(Token &Tok, llvm::ArrayRef<Token> AltTokens,
+                             unsigned int *const AltCurTokenIdx) {
+
+  // FIXME: This is a ugly hack to minimize changes for review.  This function
+  // should have this functionality factored out into a common function that
+  // takes these following as arguments either before or after this commit.
+  const Token *const Tokens =
+      AltTokens.size() ? AltTokens.data() : this->Tokens;
+  assert(!AltTokens.size() ||
+         AltCurTokenIdx &&
+             "Must specify AltCurTokenIdx if providing AltTokens");
+  unsigned int &CurToken = AltCurTokenIdx ? *AltCurTokenIdx : this->CurToken;
+
+  auto IsAtEnd = [&CurToken, &AltTokens, this] {
+    return !AltTokens.size() ? this->isAtEnd() : CurToken == AltTokens.size();
+  };
   // MSVC: If previous token was pasted, this must be a recovery from an invalid
   // paste operation. Ignore spaces before this token to mimic MSVC output.
   // Required for generating valid UUID strings in some MS headers.

     PasteOpLoc = Tokens[CurToken].getLocation();
     if (Tokens[CurToken].is(tok::hashhash))
       ++CurToken;
-    assert(!isAtEnd() && "No token on the RHS of a paste operator!");
+    assert(!IsAtEnd() && "No token on the RHS of a paste operator!");
 
     // Get the RHS token.
     const Token &RHS = Tokens[CurToken];

     // Finally, replace LHS with the result, consume the RHS, and iterate.
     ++CurToken;
     Tok = Result;
-  } while (!isAtEnd() && Tokens[CurToken].is(tok::hashhash));
+  } while (!IsAtEnd() && Tokens[CurToken].is(tok::hashhash));
 
   SourceLocation EndLoc = Tokens[CurToken - 1].getLocation();
 

test/Preprocessor/macro_vaopt_check.cpp

+// RUN: %clang_cc1 %s -Eonly -verify -Wno-all -pedantic -std=c++2a
+
+//expected-error@+1{{missing '('}}
+#define V1(...) __VA_OPT__  
+
+#undef V1
+// OK
+#define V1(...) __VA_OPT__  ()
+
+//expected-warning@+1{{can only appear in the expansion of a C99 variadic macro}}
+#define V2() __VA_OPT__(x) 
+
+//expected-error@+1{{missing ')'}}
+#define V3(...) __VA_OPT__(
+
+#define V4(...) __VA_OPT__(__VA_ARGS__)
+
+//expected-error@+1{{nested}}
+#define V5(...) __VA_OPT__(__VA_OPT__())
+
+#undef V1
+//expected-error@+1{{not followed by}}
+#define V1(...) __VA_OPT__  (#)
+
+#undef V1
+//expected-error@+1{{cannot appear at start}}
+#define V1(...) __VA_OPT__  (##)
+
+
+#undef V1
+//expected-error@+1{{cannot appear at start}}
+#define V1(...) __VA_OPT__  (## X) x
+
+#undef V1
+//expected-error@+1{{cannot appear at end}}
+#define V1(...) y __VA_OPT__  (X ##)
+
+
+#undef V1
+//expected-error@+1{{missing ')'}}
+#define V1(...) __VA_OPT__  ((())
+
+#define FOO(x,...) # __VA_OPT__(x) #x #__VA_OPT__(__VA_ARGS__) //OK
+
+#undef V1
+
+//expected-error@+1{{not followed by a macro parameter}}
+#define V1(...) __VA_OPT__(#) __VA_ARGS__
+#undef V1
+
+//expected-error@+1{{cannot appear at start}}
+#define V1(...) a __VA_OPT__(##) b
+#undef V1
+
+//expected-error@+1{{cannot appear at start}}
+#define V1(...) a __VA_OPT__(##) b __VA_OPT__(##)
+
+#undef V1
+
+#define V1(x,...) # __VA_OPT__(b x) // OK
 No newline at end of file

test/Preprocessor/macro_vaopt_expand.cpp

+// RUN: %clang_cc1 -E %s -pedantic -std=c++2a | FileCheck -strict-whitespace %s
+
+#define LPAREN ( 
+#define RPAREN ) 
+
+#define A0 expandedA0
+#define A1  expandedA1 A0
+#define A2  expandedA2 A1
+#define A3  expandedA3 A2
+
+#define A() B LPAREN )
+#define B() C LPAREN )
+#define C() D LPAREN )
+
+
+#define F(x, y) x + y 
+#define ELLIP_FUNC(...) __VA_OPT__(__VA_ARGS__)
+
+1: ELLIP_FUNC(F, LPAREN, 'a', 'b', RPAREN); 
+2: ELLIP_FUNC(F LPAREN 'a', 'b' RPAREN); 
+#undef F
+#undef ELLIP_FUNC
+
+// CHECK: 1: F, (, 'a', 'b', );
+// CHECK: 2: 'a' + 'b';
+
+#define F(...) f(0 __VA_OPT__(,) __VA_ARGS__)
+3: F(a, b, c) // replaced by f(0, a, b, c) 
+4: F() // replaced by f(0)
+
+// CHECK: 3: f(0 , a, b, c) 
+// CHECK: 4: f(0 )
+#undef F
+
+#define G(X, ...) f(0, X __VA_OPT__(,) __VA_ARGS__)
+
+5: G(a, b, c) // replaced by f(0, a , b, c) 
+6: G(a) // replaced by f(0, a) 
+7: G(a,) // replaced by f(0, a) 
+7.1: G(a,,)
+
+
+// CHECK: 5: f(0, a , b, c) 
+// CHECK: 6: f(0, a ) 
+// CHECK: 7: f(0, a ) 
+// CHECK: 7.1: f(0, a , ,)
+#undef G 
+
+#define HT_B() TONG
+
+#define F(x, ...) HT_ ## __VA_OPT__(x x A()  #x)
+
+8: F(1)
+9: F(A(),1)
+
+// CHECK: 8: HT_
+// CHECK: 9: TONG C ( ) B ( ) "A()"
+#undef HT_B
+#undef F
+
+#define F(a,...) #__VA_OPT__(A1 a)
+
+10: F(A())
+11: F(A1 A(), 1)
+// CHECK: 10: ""
+// CHECK: 11: "A1 expandedA1 expandedA0 B ( )"
+#undef F
+
+
+#define F(a,...) a ## __VA_OPT__(A1 a) ## __VA_ARGS__ ## a
+12.0: F()
+12: F(,)
+13: F(B,)
+// CHECK: 12.0: 
+// CHECK: 12: 
+// CHECK: 13: BB 
+#undef F
+
+#define F(...) #__VA_OPT__()  X ## __VA_OPT__()  #__VA_OPT__(        )
+
+14: F()
+15: F(1)
+
+// CHECK: 14: "" X ""
+// CHECK: 15: "" X ""
+
+#undef F
+
+#define SDEF(sname, ...) S sname __VA_OPT__(= { __VA_ARGS__ })
+
+16: SDEF(foo); // replaced by S foo; 
+17: SDEF(bar, 1, 2); // replaced by S bar = { 1, 2 }; 
+
+// CHECK: 16: S foo ;
+// CHECK: 17: S bar = { 1, 2 }; 
+#undef SDEF
+
+#define F(a,...) A() #__VA_OPT__(A3 __VA_ARGS__ a ## __VA_ARGS__ ## a ## C A3) A()
+
+18: F()
+19: F(,)
+20: F(,A3)
+21: F(A3, A(),A0)
+
+
+// CHECK: 18: B ( ) "" B ( ) 
+// CHECK: 19: B ( ) "" B ( ) 
+// CHECK: 20: B ( ) "A3 expandedA3 expandedA2 expandedA1 expandedA0 A3C A3" B ( )
+// CHECK: 21: B ( ) "A3 B ( ),expandedA0 A3A(),A0A3C A3" B ( )
+
+#undef F
+
+#define F(a,...) A() #__VA_OPT__(A3 __VA_ARGS__ a ## __VA_ARGS__ ## a ## C A3) a __VA_OPT__(A0 __VA_ARGS__ a ## __VA_ARGS__ ## a ## C A0) A()
+
+22: F()
+23: F(,)
+24: F(,A0)
+25: F(A0, A(),A0)
+
+
+// CHECK: 22: B ( ) "" B ( ) 
+// CHECK: 23: B ( ) "" B ( ) 
+// CHECK: 24: B ( ) "A3 expandedA0 A0C A3" expandedA0 expandedA0 A0C expandedA0 B ( )
+// CHECK: 25: B ( ) "A3 B ( ),expandedA0 A0A(),A0A0C A3" expandedA0 expandedA0 C ( ),expandedA0 A0A(),A0A0C expandedA0 B ( )
+
+#undef F
+