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

Wed Jan 27 13:51:06 PST 2021

hvdijk added a comment.

In D91913#2526117 <https://reviews.llvm.org/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 <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`.

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, I expect that hard error to remain until/unless the standard is modified to lift that restriction.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913