[PATCH] D73852: [clang] detect switch fallthrough marked by a comment (PR43465)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D73852#1872064 <https://reviews.llvm.org/D73852#1872064>, @thakis wrote:

> In D73852#1872044 <https://reviews.llvm.org/D73852#1872044>, @aaron.ballman wrote:
>
> > In D73852#1872000 <https://reviews.llvm.org/D73852#1872000>, @thakis wrote:
> >
> > > I think giving comments a semantic meaning is a bad idea. Do we really have to do this?
> >
> >
> > I think that ship has sailed, for instance, see // NOLINT comments that are also supported to silence diagnostics.
>
>
> That's for linters, not for compilers.


Yeah, I realized that later that we only support that in clang-tidy, so this is introducing a new concept to the frontend.

>> Also, the original bug report mentions that flex generates code using these constructs, so there are real world libraries using this construct.
> 
> So what? If we went down this road, we'd now have `/*OVERRIDE*/` instead of `override`, `/*FINAL*/` instead of `final`, etc. Real world libraries can be updated. That's what we already did everywhere in C++ mode. No reason it can't happen in C mode too.

This is a red herring. The feature we have is a comment that disables a diagnostic and has no semantics. You are equating that to features with semantic requirements like `override` or `final`. I don't think that's a particularly compelling argument.

> The usual progression is:
> 
> - compiler-specific extensions (we now have `__attribute__((fallthrough))` attached to an empty statement)
> - eventually, if it has legs, standardization
> 
>   This disrupts this path.

Fair points. We also add features to Clang that exist in GCC and solve real problems as part of the usual progression.

>>> In addition to this making comments have semantic meaning, the meaning is different from gcc.
>> 
>> We're covering the cases GCC does that make sense within Clang -- or have we deviated from GCC in a way that is significant to our users?
> 
> We give a not-well-specified and not documented subset of comments some meaning. We do this in a way that's different from GCC.

We can (and should!) fix up the documentation to make this more obvious. I am still not convinced we deviate from GCC in a meaningful way, but if it would make you more comfortable if we exactly match the GCC behavior identically, that seems reasonable.

> Also, as mentioned above, as-is I'd expect this patch to make builds measurably slower. That alone is revert reason enough.

As mentioned above, the author did the timing measurements at reviewer request and it did not make it measurably slower. If we have evidence that this really does add a sufficient performance regression, I totally agree that we should revert (I had the same worries). Thus far, no one has provided actual evidence that this is the case and I would not want to see a feature rejected on the hunch that it might be a performance regression.

All this said, I am comfortable reverting this back out of the 10.0 branch while we consider it harder. It does not seem so critical that we can't take time to discuss it further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73852