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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D82728#2133951 <https://reviews.llvm.org/D82728#2133951>, @Quuxplusone wrote:

> In D82728#2133720 <https://reviews.llvm.org/D82728#2133720>, @dblaikie wrote:
>
> > (the presence of at least one "override" being a signal the user intended to use override and missed some [...]
>
>
> I'm in favor of `-Wsuggest-override`, and would definitely use it if it existed.


I've no doubt a non-trivial number of folks would - we'd probably enable it in LLVM itself.

> The problem I see with `-Wmissing-override`, as it's been implemented, is that it uses the wrong granularity for "intent": it looks only across the methods of a single class, rather than across all the classes of a single header, or across a single translation unit, or across my entire codebase. In real life, I //always// want to look across my entire codebase (excluding system headers). If //any// class in my project uses `override`, I want Clang to take that as a clear declaration of intent to use `override` throughout; I don't want Clang treating class A differently from class B. But of course Clang can't look at my whole codebase simultaneously.

Right - Clang's doing its best (well, debatable - that's what we're debating) with what it's got.

> So the next best thing is to give the user a simple way to "preload the intent flag": to say "As soon as you start processing //any// class, please act as if intent has been declared for that class." Adding `-Wsuggest-override` to my build line seems like a perfect way to implement that "preload" facility.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728