Suppress non-conforming GNU paste extension in all standard-conforming modes

[hvdijk]

Summary

The GNU token paste extension that removes the comma in , ## VA_ARGS conflicts with C99/C++11's requirements when a variadic macro has no named parameters: according to the standard, an invocation as FOO() gives it a single empty argument, and concatenation of anything with an empty argument is well-defined. For this reason, the GNU extension was already disabled in C99 standard-conforming mode. It was not yet disabled in C++11 standard-conforming mode.

The associated comment suggested that GCC keeps this extension enabled in C90/C++03 standard-conforming mode, but it actually does not, so rather than adding a check for C++ language version, this change simply removes the check for C language version.

[lebedev.ri]

The associated comment suggested that GCC keeps this extension enabled in C90/C++03 standard-conforming mode, but it actually does not, so rather than adding a check for C++ language version, this change simply removes the check for C language version.

Then the comment needs to be fixed too i would think?

[hvdijk]

In D91913#2409688, @lebedev.ri wrote:

Then the comment needs to be fixed too i would think?

It's the comment right above the code that I changed; I did fix that too.

[hvdijk]

Ping

[hvdijk]

Ping 2

[hvdijk]

Ping 3

[hvdijk]

Ping 4

[rsmith]

Looks like this change matches the behavior of GCC all the way back to at least GCC 4.1.

[hvdijk]

In D91913#2518448, @rsmith wrote:

Looks like this change matches the behavior of GCC all the way back to at least GCC 4.1.

Yes, it's worked this way in GCC ever since it was first introduced back when 3.3 was in development: https://github.com/gcc-mirror/gcc/commit/58551c2335b3f2f16c8341d3bb409d37f8715696.

[dmajor]

We have a downstream build break due to this commit. One of our files has some convoluted arg-counting logic that now returns a different count, which does not match gcc: https://godbolt.org/z/W8caMr (Note: At time of writing, the clang trunk on godbolt doesn't yet have this change.)

IIUC the 12.0 release branch will be created tomorrow. If this isn't something that can be fixed quickly, please consider reverting until after the branch point.

[hvdijk]

In D91913#2520244, @dmajor wrote:

We have a downstream build break due to this commit. One of our files has some convoluted arg-counting logic that now returns a different count, which does not match gcc: https://godbolt.org/z/W8caMr (Note: At time of writing, the clang trunk on godbolt doesn't yet have this change.)

I get int nArgs = 1; with GCC and with clang (with this change) in -std=c++11 mode, I get int nArgs = 0; with GCC and with clang (with this change) in -std=gnu++11 mode and in default mode. Which command line options are needed to get output where GCC and clang differ?

[dmajor]

The bot that reported this failure was using: clang -cc1 -triple x86_64-pc-windows-msvc19.16.27038 -fms-extensions -fms-compatibility -fms-compatibility-version=19.16.27038 -std=c++17

[dmajor]

On closer inspection (sorry, I'm juggling too many things this morning) it seems gcc does give a matching int nArgs = 1 under -std=c++17 mode.

So potentially the user code is wrong? One thing that puzzles me is why this only broke on our Windows bots. I am not clear if it has to do with the MS compatibility switches or whether coincidentally some platform ifdefs in the file prevented this line from being included on other platforms.

[hvdijk]

Okay, GCC enables the standards-mandated behaviour with -std=c++17 -fms-extensions as well, but GCC doesn't have any -fms-compatibility option to compare. It makes sense to me for clang -std=c++17 -fms-compatibility to match MSVC, and MSVC supports this GCC extension even in those cases where it conflicts with the standard, so I'll try to submit a patch to restore this, conditional on -fms-compatibility, later today.

[zequanwu]

This change also breaks many chrome ToT bots(not just windows bot), example build: https://ci.chromium.org/ui/p/chrome/builders/ci/ToTLinuxOfficial/10524/steps?succeeded=true&debug=false

[hvdijk]

In D91913#2520399, @zequanwu wrote:

This change also breaks many chrome ToT bots(not just windows bot), example build: https://ci.chromium.org/ui/p/chrome/builders/ci/ToTLinuxOfficial/10524/steps?succeeded=true&debug=false

That requires an account to see.

[zequanwu]

In D91913#2520414, @hvdijk wrote:

In D91913#2520399, @zequanwu wrote:

This change also breaks many chrome ToT bots(not just windows bot), example build: https://ci.chromium.org/ui/p/chrome/builders/ci/ToTLinuxOfficial/10524/steps?succeeded=true&debug=false

That requires an account to see.

This should be accessible without an account: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8857139118288337920/+/steps/compile/0/stdout

[hvdijk]

In D91913#2520432, @zequanwu wrote:

In D91913#2520414, @hvdijk wrote:

In D91913#2520399, @zequanwu wrote:

This change also breaks many chrome ToT bots(not just windows bot), example build: https://ci.chromium.org/ui/p/chrome/builders/ci/ToTLinuxOfficial/10524/steps?succeeded=true&debug=false

That requires an account to see.

This should be accessible without an account: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8857139118288337920/+/steps/compile/0/stdout

Indeed. However, here, this seems to be a user problem. Chrome -std=c++* when building with clang, but -std=gnu++* when building with GCC for exactly this reason, see build/config/compiler/BUILD.gn. Chrome should use -std=gnu++* with clang too.

[thakis]

In D91913#2520503, @hvdijk wrote:

In D91913#2520432, @zequanwu wrote:

In D91913#2520414, @hvdijk wrote:

In D91913#2520399, @zequanwu wrote:

This change also breaks many chrome ToT bots(not just windows bot), example build: https://ci.chromium.org/ui/p/chrome/builders/ci/ToTLinuxOfficial/10524/steps?succeeded=true&debug=false

That requires an account to see.

This should be accessible without an account: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8857139118288337920/+/steps/compile/0/stdout

Indeed. However, here, this seems to be a user problem. Chrome -std=c++* when building with clang, but -std=gnu++* when building with GCC for exactly this reason, see build/config/compiler/BUILD.gn. Chrome should use -std=gnu++* with clang too.

We (and every other project out there) needs some incremental rollout plan for this. We don't want to allow _more_ GNU features. Please put this behind a -f toggle, or revert this for a bit implementing this takes some time.

[hvdijk]

In D91913#2521031, @thakis wrote:

We (and every other project out there) needs some incremental rollout plan for this.

Cut the hyperbole please, this change does not affect "every other project out there". This change makes clang match GCC's 15+-year behaviour that other projects out there will have come to expect. I find it baffling that Chrome/Chromium developers would rely on a bug, knowing full well it's a bug judging from their own comments, and then not take responsibility for the breakage when that bug is fixed. However, I'll defer to @rsmith on whether this is sufficient reason for reverting, I do not feel qualified to make that judgement.

The MSVC compatibility, however, is something I hadn't foreseen and had just submitted a fix for, D95392. If this change is to be reverted, I'll withdraw that followup fix for now.

[dmajor]

Our bots are green at b43c26d036dc. Many thanks @hvdijk for the quick response.

[rsmith]

In D91913#2521031, @thakis wrote:

In D91913#2520503, @hvdijk wrote:

Chrome -std=c++* when building with clang, but -std=gnu++* when building with GCC for exactly this reason, see build/config/compiler/BUILD.gn. Chrome should use -std=gnu++* with clang too.

We (and every other project out there) needs some incremental rollout plan for this. We don't want to allow _more_ GNU features. Please put this behind a -f toggle, or revert this for a bit implementing this takes some time.

I'm definitely sympathetic to wanting to avoid further use of GNUisms sneaking into your codebase. We're in a tricky position, though, if our choices are that either we add a -f flag for this, end up maintaining fine-grained GNU language mode flags which we don't want, and have a hard time removing those flags, or you enable all GNU language changes, and have to work to avoid them being used / have a hard time ever switching back to -std=c++*. I'd be OK with the fine-grained flag if we could reasonably plan to remove after one or two Clang releases, but these things have a tendency to never go away.

We have another option: we could restore the old behavior for the Clang 12 release branch (branching tomorrow!), and add a warning to the release branch for uses of the comma paste extension in modes where we want to remove it, so that people have a release cycle to transition before we actually make the change in anger. That would probably be the most comfortable path forward, with the least time pressure all around.

However, there are a couple of additional problems:

• There doesn't seem to be any kind of feature test macro for __VA_OPT__, so people can't easily use it when available and fall back on comma paste otherwise. I've suggested to WG21 that we allow use of #ifdef __VA_OPT__ (and #ifndef __VA_OPT__ and defined(__VA_OPT__)) for this.
• We still retain the comma paste semantics in all cases if there is at least one named parameter. Historically, the rationale for that has been that omitting the final comma in such cases was ill-formed, but that is no longer the case in C++20 onwards, with the advent of __VA_OPT__. So we should be working towards removing the getNumParams() < 2 check in at least the C++20 case.

So perhaps this might be a reasonable plan:

For Clang 12:

• Undo the semantic change and add a warning on comma pastes that will change meaning on Clang 13.
• Add feature test support for __VA_OPT__ via #ifdef __VA_OPT__.
• Clearly document in the release notes that this is going to change in Clang 13, and that __VA_OPT__ should be used instead for users who don't want to enable GNU mode or -fms-compatibility.

For Clang 13:

• Disable comma paste for macros with named parameters in -std=c89 and -std=c++98 through -std=c++17; disable comma paste for all macros in -std=c++20 onwards.
• (Also add feature test support for __VA_OPT__.)

Thoughts?

[hvdijk]

Restoring the old behaviour for clang 12 may or may not help the Chrome/Chromium people: if they also regularly build with clang from git, they would need to handle this change anyway. However, if it does help, then it sounds like a good idea to me.

So we should be working towards removing the getNumParams() < 2 check in at least the C++20 case.

Right, and the same change has also been accepted for C, has it not? If it has, then also the C2x case.

It would be good to align with GCC here. I would imagine that they will want the same thing for GCC you are proposing for clang, but it would be good to make sure. Specifically: it would be good to confirm that there are no objections from GCC's side to supporting #ifdef __VA_OPT__ as well.

[rnk]

We need a solution to our problem in the short term, though: there must be a code pattern that can be used in -std=c++14 modes to accomplish what the , ## __VA_ARGS__ code pattern accomplishes with GCC extensions.

As of right now, my understanding is that __VA_OPT__ is unavailable in C++14 mode, so I can't go fix Chrome code to use it. If there is some other straightforward way to achieve the outcome, I'm open to it. Until there is, please revert this change.

[hvdijk]

@rnk Taking it upon yourself to revert this without approval and without communication on all branches, especially given the earlier suggestion by @rsmith to only revert this on the LLVM 12 branch, is an abuse of your commit privileges as far as I am concerned. This is not a Google project. Google does not get to unilaterally dictate its direction. I do not intend to get into a revert-the-revert war and will not revert your revert myself. I do expect you to do so.

[rsmith]

In D91913#2525993, @hvdijk wrote:

@rnk Taking it upon yourself to revert this without approval and without communication on all branches, especially given the earlier suggestion by @rsmith to only revert this on the LLVM 12 branch, is an abuse of your commit privileges as far as I am concerned. This is not a Google project. Google does not get to unilaterally dictate its direction. I do not intend to get into a revert-the-revert war and will not revert your revert myself. I do expect you to do so.

I think @rnk's observation that __VA_OPT__ isn't actually available in any compilation mode other than C++20 (which I hadn't previously realized) is important here: we'd left longstanding users of this functionality with no path forward, and that is, I think, sufficient motivation for a revert. I'm working on a patch to make __VA_OPT__ available across all language modes. Once that's landed, the next step should be adding the warning to trunk and the Clang 12 branch, and then we can decide how long we want the warning in trunk before we make the conformance change -- while we're not beholden to the needs of any particular Clang user, Clang doesn't exist in a vacuum, and giving our live-at-head users a little time to adapt to this change seems reasonable, especially given that there doesn't seem to be any particular urgency in this fix.

[rsmith]

rG0436ec2128c9775ba13b0308937238fc79673fdd enables __VA_OPT__ across language modes and allows support for it to be detected by #ifdef.

[rnk]

In D91913#2525993, @hvdijk wrote:

@rnk Taking it upon yourself to revert this without approval and without communication on all branches, especially given the earlier suggestion by @rsmith to only revert this on the LLVM 12 branch, is an abuse of your commit privileges as far as I am concerned. This is not a Google project. Google does not get to unilaterally dictate its direction. I do not intend to get into a revert-the-revert war and will not revert your revert myself. I do expect you to do so.

I'm sorry you feel that way, but I do believe that I have communicated the issue clearly already. I don't mean to say that Google code is more important than any other code. We can update our code as long as there is a portable conforming way to do it. I think what I did is reasonable, and falls under the situations outlined here:
https://llvm.org/docs/CodeReview.html#can-code-be-reviewed-after-it-is-committed

This , ## __VA_ARG__ extension serves a valid use case. The C++20 standard __VA_OPT__ feature shows that the committee recognizes the validity of this use case. As things stood before the revert, there was no way for a user of this feature to fix their code so that it would compile under -std=c++17, 14, 11, etc. My commit message attempted to outline what was necessary to reland.

In D91913#2526171, @rsmith wrote:

rG0436ec2128c9775ba13b0308937238fc79673fdd enables __VA_OPT__ across language modes and allows support for it to be detected by #ifdef.

Thanks. Do we expect that GCC will want to follow this behavior? I'm concerned that I might have a hard time selling the patch to use __VA_OPT__ to V8 folks, who support a wide variety of compilers. The patch will not be pretty, since I cannot put an ifdef into the middle of a variadic macro.

As an alternative, have you considered allowing the old GNU extension unless C++20 features are enabled? This would make it so that projects encounter this portability issue when they upgrade to C++20 instead of when they upgrade compilers. Maybe this has been considered, I don't have all the context.

[rsmith]

In D91913#2526255, @rnk wrote:

In D91913#2525993, @hvdijk wrote:

@rnk Taking it upon yourself to revert this without approval and without communication on all branches, especially given the earlier suggestion by @rsmith to only revert this on the LLVM 12 branch, is an abuse of your commit privileges as far as I am concerned. This is not a Google project. Google does not get to unilaterally dictate its direction. I do not intend to get into a revert-the-revert war and will not revert your revert myself. I do expect you to do so.

I'm sorry you feel that way, but I do believe that I have communicated the issue clearly already. I don't mean to say that Google code is more important than any other code. We can update our code as long as there is a portable conforming way to do it. I think what I did is reasonable, and falls under the situations outlined here:
https://llvm.org/docs/CodeReview.html#can-code-be-reviewed-after-it-is-committed

This , ## __VA_ARG__ extension serves a valid use case. The C++20 standard __VA_OPT__ feature shows that the committee recognizes the validity of this use case. As things stood before the revert, there was no way for a user of this feature to fix their code so that it would compile under -std=c++17, 14, 11, etc. My commit message attempted to outline what was necessary to reland.

In D91913#2526171, @rsmith wrote:

rG0436ec2128c9775ba13b0308937238fc79673fdd enables __VA_OPT__ across language modes and allows support for it to be detected by #ifdef.

Thanks. Do we expect that GCC will want to follow this behavior?

Looks like they will change to allowing __VA_OPT__ across language modes at least: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98859

I'm concerned that I might have a hard time selling the patch to use __VA_OPT__ to V8 folks, who support a wide variety of compilers. The patch will not be pretty, since I cannot put an ifdef into the middle of a variadic macro.

You can factor the decision out:

#define THIRD_ARG(a, b, c, ...) c
#define HAS_VA_OPT(...) THIRD_ARG(__VA_OPT__(,), 1, 0, )
#if HAS_VA_OPT(?)
#define MAYBE_COMMA(...) __VA_OPT__(, __VA_ARGS__)
#else
#define MAYBE_COMMA(...) , ##__VA_ARGS__
#endif
#undef HAS_VA_OPT
#undef THIRD_ARG

#define MY_VARIADIC_MACRO(X, ...) printf(X MAYBE_COMMA(__VA_ARGS__))

I've reverted the change to permit #ifdef __VA_OPT__, because it's unfortunately not actually useful: older versions of Clang and GCC and ICC reject #ifdef __VA_OPT__ because the language rules only permit the __VA_OPT__ token to appear in the right-hand side of a variadic macro. So you'll need something like the above to detect the existence of __VA_OPT__.

As an alternative, have you considered allowing the old GNU extension unless C++20 features are enabled? This would make it so that projects encounter this portability issue when they upgrade to C++20 instead of when they upgrade compilers. Maybe this has been considered, I don't have all the context.

This isn't a C++20 problem; the old behavior was non-conforming across all language modes with variadic macros. In particular:

#define FOO(...) , ## __VA_ARGS__
FOO()

... is required to expand to a comma in C99 onwards and C++11 onwards, but was incorrectly expanding to an empty token sequence.

C++20 adds a new twist here, in that

#define BAR(a, ...) , ## __VA_ARGS__
BAR(a)

... now has the same problem. This is invalid in C++17 and earlier and in C11 and earlier: a use of BAR was required to contain at least one comma, but that rule was relaxed when __VA_OPT__ was added in C++20. And that makes the , ## __VA_ARGS__ behavior no longer a conforming extension. @hvdijk's patch only changed the behavior of the first of these two examples, but in the longer term, we'll want to change the behavior of the second example too, at least in C++20 onwards.

[hvdijk]

In D91913#2526117, @rsmith wrote:

I think @rnk's observation that __VA_OPT__ isn't actually available in any compilation mode other than C++20 (which I hadn't previously realized) is important here: we'd left longstanding users of this functionality with no path forward, and that is, I think, sufficient motivation for a revert.

My concern isn't with the revert itself, it's without waiting for approval. That's a crystal clear LLVM developer policy violation: changes need to be approved, or obvious, or by developers responsible for the code being modified. @rnk himself has linked to the page making this clear, just before the bit he linked to: https://llvm.org/docs/CodeReview.html#must-code-be-reviewed-prior-to-being-committed. The fact that reviews don't close after committing does not change that.

"Longstanding users" is not right: there has not been any indication that this affected anyone other than Google. (Except for the MSVC compatibility issue.)
"No path forward" is not right either: Google already had multiple paths forward.

• Google could have reverted this on the LLVM 12 branch and worked constructively to help resolve this on the main branch in a way that also works for them.
• Google could have temporarily switched to -std=gnu++* and worked constructively to help resolve this on both branches in a way that also works for them.
• Google could modify their own LLVM version and use -std=c++* on that version, or -std=gnu++* with unmodified LLVM. (I would not suggest this for ordinary users, but my understanding is that Google already have their own version of clang that they normally use. My understanding may be wrong.)
• Google could have chosen to always build in -std=c++20 mode.

And most importantly:

• Google could have fixed their code. I've looked at how this is used. I'm about 80-90% sure it's possible to change the macros so that this extension is no longer needed, *without* using __VA_OPT__, with an admittedly very ugly use of the preprocessor. That ugly use could be conditional on the language version, using a clean __VA_OPT__ version in C++20 mode, and the ugly version in older modes.

In D91913#2526171, @rsmith wrote:

rG0436ec2128c9775ba13b0308937238fc79673fdd enables __VA_OPT__ across language modes and allows support for it to be detected by #ifdef.

I've since found that C++20 requires a diagnostic for #ifdef __VA_OPT__: it violates [cpp.replace]p6: "The identifiers VA_ARGS and VA_OPT shall occur only in the replacement-list of a function-like macro that uses the ellipsis notation in the parameters." It is a hard error with GCC under -pedantic-errors and given that this hard error is required by the standard (edit: this diagnostic is required by the standard and -pedantic-errors by design makes all standard-mandated diagnostics hard errors), I expect that hard error to remain until/unless the standard is modified to lift that restriction.

[rnk]

OK, thanks for the direction. We'll try to port code to use the device you listed.

I see you think we need a new warning for this, a clang 12 release note, and then this can reland. @hvdijk , that plan looks good to me, and I'll let you and Richard sort it out. I don't plan to reland this change, please reland when appropriate.

[rnk]

In D91913#2526317, @hvdijk wrote:

My concern isn't with the revert itself, it's without waiting for approval. That's a crystal clear LLVM developer policy violation: changes need to be approved, or obvious, or by developers responsible for the code being modified. @rnk himself has linked to the page making this clear, just before the bit he linked to: https://llvm.org/docs/CodeReview.html#must-code-be-reviewed-prior-to-being-committed. The fact that reviews don't close after committing does not change that.

It is my understanding that it does not require approval to revert a patch if a reviewer makes comments that were not addressed post-review. I think the linked wording supports that interpretation. Several of us raised concerns about this patch after it landed, and our concerns remained unaddressed when I reverted the patch.

"Longstanding users" is not right: there has not been any indication that this affected anyone other than Google. (Except for the MSVC compatibility issue.)
"No path forward" is not right either: Google already had multiple paths forward.

I'm mainly responsible for Chromium, so I will take "Google" to mean "Chromium" here.

• Google could have reverted this on the LLVM 12 branch and worked constructively to help resolve this on the main branch in a way that also works for them.

We don't use the release branches, we do our own releases periodically from head.

• Google could have temporarily switched to -std=gnu++* and worked constructively to help resolve this on both branches in a way that also works for them.

Switching modes would've allowed the use of other gnu extensions, which we do not permit, and which we would've had to clean up later when switching back to -std=c++*.

• Google could modify their own LLVM version and use -std=c++* on that version, or -std=gnu++* with unmodified LLVM. (I would not suggest this for ordinary users, but my understanding is that Google already have their own version of clang that they normally use. My understanding may be wrong.)

We don't currently have infrastructure to make such local modifications. Nothing is impossible, of course, we could do as you suggest, but we try our best to always release a version of upstream LLVM tools.

• Google could have chosen to always build in -std=c++20 mode.

This isn't feasible, we still can't build in C++17 mode, and there are many blockers to migrating.

And most importantly:

• Google could have fixed their code. I've looked at how this is used. I'm about 80-90% sure it's possible to change the macros so that this extension is no longer needed, *without* using __VA_OPT__, with an admittedly very ugly use of the preprocessor. That ugly use could be conditional on the language version, using a clean __VA_OPT__ version in C++20 mode, and the ugly version in older modes.

If that's the case, I believe I asked for options. I'm open to suggestions, and I'm not trying to leave you with a passive-aggressive "patches welcome" offer where you do all the work. I'm truly not aware of how we would make this code conforming. Maybe there is a way that I'm unaware of.

---

Anyway, I apologize for the misunderstanding. I'm doing my best to operate in good faith with LLVM project policies. Hopefully you feel that you have a path forward here.

[rsmith]

In D91913#2526317, @hvdijk wrote:

In D91913#2526117, @rsmith wrote:

I think @rnk's observation that __VA_OPT__ isn't actually available in any compilation mode other than C++20 (which I hadn't previously realized) is important here: we'd left longstanding users of this functionality with no path forward, and that is, I think, sufficient motivation for a revert.

My concern isn't with the revert itself, it's without waiting for approval. That's a crystal clear LLVM developer policy violation: changes need to be approved, or obvious, or by developers responsible for the code being modified. @rnk himself has linked to the page making this clear, just before the bit he linked to: https://llvm.org/docs/CodeReview.html#must-code-be-reviewed-prior-to-being-committed. The fact that reviews don't close after committing does not change that.

Commits and reverts are not held to the same standards. We expect changes to be reverted early, and generally encourage patches to be reverted as a first response if problems are discovered (soon) after the original commit. This is all part of keeping trunk clean, which is to everyone's benefit.

"Longstanding users" is not right: there has not been any indication that this affected anyone other than Google. (Except for the MSVC compatibility issue.)
"No path forward" is not right either: Google already had multiple paths forward.

Problems with early adopters are signs that others will have problems too. And none of the paths forward that you mentioned are really reasonable and proportionate, given the small scale of the problem that this change addresses.

Fundamentally, while we *can* change Clang trunk to do anything we like, we shouldn't: we need to have respect for our users, their time, and their existing codebases. And that means making changes like this in a responsible fashion, giving notice, and responding well when we hear that the change has caused problems in practice. This change is not urgent, and by moving more slowly, we can significantly reduce the cost on our users of adapting to it.

[hvdijk]

In D91913#2526403, @rnk wrote:

Anyway, I apologize for the misunderstanding. I'm doing my best to operate in good faith with LLVM project policies. Hopefully you feel that you have a path forward here.

Thank you, I appreciate that.

If that's the case, I believe I asked for options. I'm open to suggestions, and I'm not trying to leave you with a passive-aggressive "patches welcome" offer where you do all the work. I'm truly not aware of how we would make this code conforming. Maybe there is a way that I'm unaware of.

Please take a look at Jens Gustedt's "Detect empty macro arguments": https://gustedt.wordpress.com/2010/06/08/detect-empty-macro-arguments/

It cannot function as a general solution for all possible uses of , ## __VA_ARGS__, because it fails to handle some cases that users of , ## __VA_ARGS__ do expect to be handled (more than listed in that blog post), but from what I have seen it should handle what Chromium uses. This can either be adapted to directly expand to what Chromium needs, or can be used combined with two other macros can be defined, call them A_0 and A_1 for simplicity, and CONCAT(A_, ISEMPTY(__VA_ARGS__))(__VA_ARGS__) (with the obvious definition of CONCAT) would expand either A_0(__VA_ARGS__) or A_1(__VA_ARGS__), where A_1 would leave out a comma that A_0 would include.

[dmajor]

If I'm reading git correctly, the change is still present on the 12.x branch. Should it be reverted there too?

[hvdijk]

In D91913#2526866, @dmajor wrote:

If I'm reading git correctly, the change is still present on the 12.x branch. Should it be reverted there too?

I could have sworn that I saw it already reverted on the 12.x branch too, but I don't know what it was that I was looking at. It doesn't make sense to me to leave it applied on 12.x but not have it applied on main. Unless it can be reapplied on main, yes, I think it should be reverted on the 12.x branch.

And I might as well include this here for the benefit of anyone else affected by this in the future: adapting Jens Gustedt's trick just a little bit, given the below, MAYBE_COMMA(__VA_ARGS__) __VA_ARGS__ is pretty much a drop-in replacement for __VA_OPT__(,) __VA_ARGS__, assuming no odd macro arguments are used. It's not exactly the same thing as , ## __VA_ARGS__, but generally close enough.

#define ARG128(                                  \
   _0,   _1,   _2,   _3,   _4,   _5,   _6,   _7, \
   _8,   _9,  _10,  _11,  _12,  _13,  _14,  _15, \
  _16,  _17,  _18,  _19,  _20,  _21,  _22,  _23, \
  _24,  _25,  _26,  _27,  _28,  _29,  _30,  _31, \
  _32,  _33,  _34,  _35,  _36,  _37,  _38,  _39, \
  _40,  _41,  _42,  _43,  _44,  _45,  _46,  _47, \
  _48,  _49,  _50,  _51,  _52,  _53,  _54,  _55, \
  _56,  _57,  _58,  _59,  _60,  _61,  _62,  _63, \
  _64,  _65,  _66,  _67,  _68,  _69,  _70,  _71, \
  _72,  _73,  _74,  _75,  _76,  _77,  _78,  _79, \
  _80,  _81,  _82,  _83,  _84,  _85,  _86,  _87, \
  _88,  _89,  _90,  _91,  _92,  _93,  _94,  _95, \
  _96,  _97,  _98,  _99, _100, _101, _102, _103, \
 _104, _105, _106, _107, _108, _109, _110, _111, \
 _112, _113, _114, _115, _116, _117, _118, _119, \
 _120, _121, _122, _123, _124, _125, _126, _127, \
 ...) _127

#define IF_HAS_COMMA(a, b, ...) \
 /* Expands to a if __VA_ARGS__ contains a comma, b otherwise. */ \
 ARG128(__VA_ARGS__                               \
  a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
  a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
  a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
  a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
  a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
  a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
  a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
  a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, b,)

#define CONCAT_5_(a, b, c, d, e) a ## b ## c ## d ## e
#define CONCAT_5(a, b, c, d, e) CONCAT_5_(a, b, c, d, e)

#define COMMA(...) ,
#define EMPTY(...)

#define MAYBE_COMMA_1000 ,
#define MAYBE_COMMA(...) \
  /* Expands to , if __VA_ARGS__ is non-empty, nothing otherwise. */ \
  IF_HAS_COMMA(EMPTY, COMMA, CONCAT_5(MAYBE_COMMA_, \
    IF_HAS_COMMA(1, 0, COMMA __VA_ARGS__ ()),       \
    IF_HAS_COMMA(1, 0,       __VA_ARGS__   ),       \
    IF_HAS_COMMA(1, 0, COMMA __VA_ARGS__   ),       \
    IF_HAS_COMMA(1, 0,       __VA_ARGS__ ())))()

clang/lib/Lex/TokenLexer.cpp

bool TokenLexer::MaybeRemoveCommaBeforeVaArgs(
 
   // In Microsoft-compatibility mode, a comma is removed in the expansion
   // of " ... , __VA_ARGS__ " if __VA_ARGS__ is empty.  This extension is
   // not supported by gcc.
   if (!HasPasteOperator && !PP.getLangOpts().MSVCCompat)
     return false;
 
   // GCC removes the comma in the expansion of " ... , ## __VA_ARGS__ " if
-  // __VA_ARGS__ is empty, but not in strict C99 mode where there are no
-  // named arguments, where it remains.  In all other modes, including C99
-  // with GNU extensions, it is removed regardless of named arguments.
+  // __VA_ARGS__ is empty, but not in strict mode where there are no
+  // named arguments, where it remains.  With GNU extensions, it is removed
+  // regardless of named arguments.
   // Microsoft also appears to support this extension, unofficially.
-  if (PP.getLangOpts().C99 && !PP.getLangOpts().GNUMode
-        && Macro->getNumParams() < 2)
+  if (!PP.getLangOpts().GNUMode && Macro->getNumParams() < 2)
     return false;
 
   // Is a comma available to be removed?
   if (ResultToks.empty() || !ResultToks.back().is(tok::comma))
     return false;
 
   // Issue an extension diagnostic for the paste operator.
   if (HasPasteOperator)

clang/test/Preprocessor/macro_fn_comma_swallow2.c

 // Test the __VA_ARGS__ comma swallowing extensions of various compiler dialects.
 
 // RUN: %clang_cc1 -E %s | FileCheck -check-prefix=GCC -strict-whitespace %s
+// RUN: %clang_cc1 -E -std=c90 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c99 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c11 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -x c++ %s | FileCheck -check-prefix=GCC -strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++03 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++11 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=gnu99 %s | FileCheck -check-prefix=GCC -strict-whitespace %s
 // RUN: %clang_cc1 -E -fms-compatibility %s | FileCheck -check-prefix=MS -strict-whitespace %s
 // RUN: %clang_cc1 -E -DNAMED %s | FileCheck -check-prefix=GCC -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c99 -DNAMED %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 
 
 #ifndef NAMED
 # define A(...)   [ __VA_ARGS__ ]