[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

Wed Jan 27 13:35:29 PST 2021

rsmith added a comment.

In D91913#2526255 <https://reviews.llvm.org/D91913#2526255>, @rnk wrote:

> In D91913#2525993 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D91913#2526171>, @rsmith wrote:
>
>> rG0436ec2128c9775ba13b0308937238fc79673fdd <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91913/new/

https://reviews.llvm.org/D91913