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

Wed Jan 27 14:09:03 PST 2021

rnk added a comment.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913