[PATCH] D82728: [clang] Add -Wsuggest-override

Tue Jul 7 00:56:15 PDT 2020

dblaikie added a comment.

In D82728#2135142 <https://reviews.llvm.org/D82728#2135142>, @logan-5 wrote:

> Glad this is generating some discussion. For my $0.02, I would also (obviously) love to be able to enable this warning on all the codebases I work on, and this patch was born out of a discussion on the C++ Slack with another user who had found this warning very useful in GCC and was wondering why Clang didn't have it yet.
>
> In D82728#2134072 <https://reviews.llvm.org/D82728#2134072>, @dblaikie wrote:
>
> > The issue is that such a warning then needs to be off by default, because we can't assume the user's intent. And Clang's historically been fairly averse to off-by-default warnings due to low user-penetration (not zero, like I said, I imagine LLVM itself would use such a warning, were it implemented) & so concerns about the cost/benefit tradeoff of the added complexity (source code and runtime) of the feature.
>
>
> I agree `-Wsuggest-override` should be off by default, yet I suspect its user-penetration will be much higher than other off-by-default warnings, due to numerous instances of people on the Internet asking for <https://stackoverflow.com/a/41109550/5379590> this feature <https://stackoverflow.com/a/29154136/5379590>, as well as the precedent for it set by GCC.

Ah, I hadn't checked/wasn't mentioned whether GCC has this warning. If GCC already has it, there's usually an easy enough argument to be made for GCC compatibility being worthwhile that overcomes most of the objections unless the GCC warning is particularly problematic in some way that I doubt this is.

Is the implementation you're proposing fairly consistent with GCC's? Run it over any big codebases to check it warns in the same places GCC does?

> Moreover, since this implementation of this warning lies along the exact same code paths as the already existing `-Winconsistent-missing-override`, the added complexity from this patch is absolutely minimal.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728