[Clang] Fix __VA_OPT__ implementation so that it treats the concatenation of a non-placemaker token and placemaker token as a non-placemaker token

[shafik]

Summary

Currently the implementation of __VA_OPT__ will treat the concatenation of a non-placemaker token and placemaker token as a placemaker token which is not correct. This will fix the implementation and treat the result as a non-placemaker token.

This fixes: https://github.com/llvm/llvm-project/issues/60268

[shafik]

• Update line ending in test

[shafik]

• I think I correctly updated line endings this time

[shafik]

clang/test/Preprocessor/macro_vaopt_p1042r1.cpp

1

I removed Windows lines ending from this test, I don't think they matter but if someone can see why they would let me know.

[shafik]

• Apply clang-format

[shafik]

It looks like the modules test failures are unrelated and are failing all the recent modules builds.

[shafik]

Ping

[erichkeane]

Pretty sure @cor3ntin and @tahonermann are our Lexer code owners? Or at least were in the initial proposal.

[aaron.ballman]

LGTM but please add a release note for the fix.

[aaron.ballman]

I don't think the libc++ precommit CI failures are related to this change and so I don't think they block anything, but it'd be nice if a libc++ person could verify (CC @ldionne @Mordante )

[ldionne]

I suspect the libc++ failure is because the patch is not rebased onto main. I remember seeing those issues a while back. I think it's unlikely to be this patch.

[shafik]

• Add release note

clang/docs/ReleaseNotes.rst

- Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   (`#60344 <https://github.com/llvm/llvm-project/issues/60344>`_)
 - Fix confusing warning message when ``/clang:-x`` is passed in ``clang-cl``
   driver mode and emit an error which suggests using ``/TC`` or ``/TP``
   ``clang-cl`` options instead.
   (`#59307 <https://github.com/llvm/llvm-project/issues/59307>`_)
 - Fix assert that fails when the expression causing the this pointer to be
   captured by a block is part of a constexpr if statement's branch and
   instantiation of the enclosing method causes the branch to be discarded.
+- Fix __VA_OPT__ implementation so that it treats the concatenation of a
+  non-placemaker token and placemaker token as a non-placemaker token.
+  (`#60268 <https://github.com/llvm/llvm-project/issues/60268>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to C++ Support

clang/lib/Lex/TokenLexer.cpp

if (!PasteBefore && !PasteAfter) {
         // We're creating a placeholder token. Usually this doesn't matter,
         // but it can affect paste behavior when at the start or end of a
         // __VA_OPT__.
         if (NonEmptyPasteBefore) {
           // We're imagining a placeholder token is inserted here. If this is
           // the first token in a __VA_OPT__ after a ##, delete the ##.
           assert(VCtx.isInVAOpt() && "should only happen inside a __VA_OPT__");
           VCtx.hasPlaceholderAfterHashhashAtStart();
-        }
-        if (RParenAfter)
+        } else if (RParenAfter)
           VCtx.hasPlaceholderBeforeRParen();
       }
       continue;
     }
 
     // Okay, we have a token that is either the LHS or RHS of a paste (##)
     // argument.  It gets substituted as its non-pre-expanded tokens.
     const Token *ArgToks = ActualArgs->getUnexpArgument(ArgNo);
for (unsigned I = 0, E = NumTokens; I != E; ++I) {
     // empty.
     if (PasteAfter) {
       // Discard the argument token and skip (don't copy to the expansion
       // buffer) the paste operator after it.
       ++I;
       continue;
     }
 
-    if (RParenAfter)
+    if (RParenAfter && !NonEmptyPasteBefore)
       VCtx.hasPlaceholderBeforeRParen();
 
     // If this is on the RHS of a paste operator, we've already copied the
     // paste operator to the ResultToks list, unless the LHS was empty too.
     // Remove it.
     assert(PasteBefore);
     if (NonEmptyPasteBefore) {
       assert(ResultToks.back().is(tok::hashhash));

clang/test/Preprocessor/macro_vaopt_p1042r1.cpp

- RUN: %clang_cc1 -E %s -pedantic -std=c++2a | FileCheck -strict-whitespace %s
-
-#define LPAREN() (
-#define G(Q) 42
-#define F1(R, X, ...)  __VA_OPT__(G R X) )
-1: int x = F1(LPAREN(), 0, <:-);
-// CHECK: 1: int x = 42;
-
-#define F2(...) f(0 __VA_OPT__(,) __VA_ARGS__)
-#define EMP
-2: F2(EMP)
-// CHECK: 2: f(0 )
-
-#define H3(X, ...) #__VA_OPT__(X##X X##X)
-3: H3(, 0)
-// CHECK: 3: ""
-
-#define H4(X, ...) __VA_OPT__(a X ## X) ## b
-4: H4(, 1)
-// CHECK: 4: a b
-
-#define H4B(X, ...) a ## __VA_OPT__(X ## X b)
-4B: H4B(, 1)
-// CHECK: 4B: a b
-
-#define H5A(...) __VA_OPT__()/**/__VA_OPT__()
-#define H5B(X) a ## X ## b
-#define H5C(X) H5B(X)
-5: H5C(H5A())
-// CHECK: 5: ab
+ RUN: %clang_cc1 -E %s -pedantic -std=c++2a | FileCheck -strict-whitespace %s
+
+#define LPAREN() (
+#define G(Q) 42
+#define F1(R, X, ...)  __VA_OPT__(G R X) )
+1: int x = F1(LPAREN(), 0, <:-);
+// CHECK: 1: int x = 42;
+
+#define F2(...) f(0 __VA_OPT__(,) __VA_ARGS__)
+#define EMP
+2: F2(EMP)
+// CHECK: 2: f(0 )
+
+#define H3(X, ...) #__VA_OPT__(X##X X##X)
+3: H3(, 0)
+// CHECK: 3: ""
+
+#define H4(X, ...) __VA_OPT__(a X ## X) ## b
+4: H4(, 1)
+// CHECK: 4: a b
+
+#define H4B(X, ...) a ## __VA_OPT__(X ## X b)
+4B: H4B(, 1)
+// CHECK: 4B: a b
+
+#define H5A(...) __VA_OPT__()/**/__VA_OPT__()
+#define H5B(X) a ## X ## b
+#define H5C(X) H5B(X)
+5: H5C(H5A())
+// CHECK: 5: ab
+
+namespace GH60268 {
+#define H6(X, ...) __VA_OPT__(a ## X) ## b
+6: H6(, 1);
+// CHECK: 6: ab
+}