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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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 :)


Repository:
  rL LLVM

https://reviews.llvm.org/D35782