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

Richard Smith - zygoloid via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Aug 23 17:20:41 PDT 2017

rsmith added a comment.

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.

Comment at: include/clang/Basic/DiagnosticLexKinds.td:360
+def err_vaopt_paste_at_end : Error<"'##' cannot appear at end within __VA_OPT__">;
`'##' cannot appear at end of __VA_OPT__ argument` please, to match the change to the previous diagnostic.

Comment at: include/clang/Lex/VariadicMacroSupport.h:1
+//===- VariadicMacroSupport.h - state-machines and scope-gaurds -*- C++ -*-===//
rsmith wrote:
> Typo "gaurds".
nit: "state machines" and "scope guards" should not be hyphenated

Comment at: include/clang/Lex/VariadicMacroSupport.h:69
+  class VAOptDefinitionContext {
+    Preprocessor &PP;
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."

Comment at: include/clang/Lex/VariadicMacroSupport.h:119
+  class VAOptExpansionContext : VAOptDefinitionContext {
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."

Comment at: lib/Lex/PPDirectives.cpp:2384-2418
+        if (VAOCtx.isVAOptToken(Tok)) {
+          // If we're already within a VAOPT, emit an error.
+          if (VAOCtx.isInVAOpt()) {
+            Diag(Tok, diag::err_pp_vaopt_nested_use);
+            return nullptr;
+          }
+          // Ensure VAOPT is followed by a '(' .
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, ...

Comment at: lib/Lex/Preprocessor.cpp:130
+  if (getLangOpts().CPlusPlus2a) {
+    (Ident__VA_OPT__ = getIdentifierInfo("__VA_OPT__"))->setIsPoisoned();
+    SetPoisonReason(Ident__VA_OPT__,diag::ext_pp_bad_vaopt_use);
Have you considered allowing this as an extension in other language modes?

Comment at: lib/Lex/TokenLexer.cpp:189-195
+  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;
Please make this a member function of `MacroArgs` (passing in the `MacroInfo*`).

Comment at: lib/Lex/TokenLexer.cpp:275-284
+            // If VAOPT had a hashhash before it (that was itself not preceded
+            // by a placemarker token and therefore added to ResultToks) and if 
+            // VAOPT reduces to a placemarker, remove that hashhash.
+            // 
+            // #define F(a,...) a ## __VA_OPT__(x)
+            //   F()  <-- ResultToks does not contain preceding hashhash.
+            //   F(1) <-- ResultToks does contain the preceding hashhash.
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.

Comment at: lib/Lex/TokenLexer.cpp:285
+              ResultToks.pop_back();
+          } else if (HasPasteAfter && !VCtx.hasStringifyOrCharifyBefore()) {
+            ++I; // Skip the hashhash if the empty tokens are not stringified
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`.

Comment at: lib/Lex/TokenLexer.cpp:321-322
+              // Replace the token prior to the first ## in this iteration.
+              ConcatenatedVAOPTResultToks[ConcatenatedVAOPTResultToks.size() -
+                                          1] = LHS;
+              if (CurTokenIdx == NumVAOptTokens)
`ConcatenatedVAOPTResultToks.back() = LHS;`

Comment at: lib/Lex/TokenLexer.cpp:357
+        break /*out of inner loop*/;
+      } while (!CalledWithVariadicArguments && I != E);  
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.)

Comment at: lib/Lex/TokenLexer.cpp:418-420
+           "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");
This seems excessive. Just "unexpected ## in result tokens" would do, I think.

Comment at: lib/Lex/TokenLexer.cpp:572-575
+      // Do not remove the paste operator if it is the one before __VA_OPT__
+      // (and we are still processing tokens within VA_OPT).  We handle the case
+      // of removing the paste operator if __VA_OPT__ reduces to the notional
+      // placemarker above when we encounter the closing paren of VA_OPT.
This is a fun corner case. :)

Comment at: lib/Lex/TokenLexer.cpp:723-725
+  // 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.
Please go ahead and do that refactoring now :)



More information about the cfe-commits mailing list