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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
