[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.

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



================
Comment at: include/clang/Basic/DiagnosticLexKinds.td:351
+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<
----------------
Seems weird to mention C99 in a diagnostic about a C++20 feature. Maybe just drop the "C99" here?


================
Comment at: include/clang/Basic/DiagnosticLexKinds.td:355
+def err_pp_missing_rparen_in_vaopt_use : Error<
+  "missing ')' following __VA_OPT__( contents ">;
+def err_pp_vaopt_nested_use : Error<
----------------
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 '('?


================
Comment at: include/clang/Basic/DiagnosticLexKinds.td:360
+def err_vaopt_paste_at_start : Error<
+  "'##' cannot appear at start within __VA_OPT__">;
+
----------------
"at start within __VA_OPT__" might be clearer as "at start of __VA_OPT__ argument" or similar.


================
Comment at: include/clang/Lex/Preprocessor.h:133
   IdentifierInfo *Ident__VA_ARGS__;                // __VA_ARGS__
+  IdentifierInfo *Ident__VA_OPT__;                 // __VA_OPT__(contents)
   IdentifierInfo *Ident__has_feature;              // __has_feature
----------------
Drop the "(contents)" from the comment here. The identifier we're tracking here is just `__VA_OPT__`.


================
Comment at: include/clang/Lex/VariadicMacroSupport.h:1
+//===- VariadicMacroSupport.h - state-machines and scope-gaurds -*- C++ -*-===//
+//
----------------
Typo "gaurds".


================
Comment at: include/clang/Lex/VariadicMacroSupport.h:24-27
+  class VariadicMacroScopeGuard {
+    const Preprocessor &PP;
+    IdentifierInfo &Ident__VA_ARGS__;
+    IdentifierInfo &Ident__VA_OPT__;
----------------
Please factor out this class (for the `__VA_ARGS__` case) and commit it separately.


================
Comment at: include/clang/Lex/VariadicMacroSupport.h:30-31
+    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...
----------------
Don't look these up by string each time, instead grab the value cached on the preprocessor.


================
Comment at: include/clang/Lex/VariadicMacroSupport.h:35
+             "__VA_ARGS__ should be poisoned!");
+      assert(!PP.getLangOpts().CPlusPlus2a ||
+             (Ident__VA_OPT__.isPoisoned() &&
----------------
Rather than repeating this connection between language mode and this particular feature here, maybe check whether the cached `IdentifierInfo*` on the preprocessor is null?


================
Comment at: include/clang/Lex/VariadicMacroSupport.h:57-59
+      OUTSIDE_VAOPT = -1,
+      MATCHED_CLOSING_PARENS_OF_VAOPT = 0,
+      MATCHED_OPENING_PARENS_OF_VAOPT = 1
----------------
LLVM / Clang style uses CamelCase for names such as this.


================
Comment at: include/clang/Lex/VariadicMacroSupport.h:61
+    };
+    int State = OUTSIDE_VAOPT;
+    const IdentifierInfo *const Ident__VA_OPT__;
----------------
This should not be called `State` if what it's tracking is the number of nested parens. Please give this a better name.


================
Comment at: include/clang/Lex/VariadicMacroSupport.h:74
+    
+    // enter - call this as soon as you see __VA_OPT__ and '('
+    void enter() {
----------------
Doc comments should start with three slashes, not repeat the function name, and be formatted as a sentence (leading capital letter, terminating period).


================
Comment at: lib/Lex/TokenLexer.cpp:192
+      const int VariadicArgIndex = Macro->getNumParams() - 1;
+      const Token *VarArgs = ActualArgs->getUnexpArgument(VariadicArgIndex);
+      return VarArgs->isNot(tok::eof);
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D35782