[PATCH] D82617: Disable GCC's -Woverloaded-virtual, which has false positives.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D82617#2118253 <https://reviews.llvm.org/D82617#2118253>, @sammccall wrote:

> In D82617#2118118 <https://reviews.llvm.org/D82617#2118118>, @dblaikie wrote:
>
> > In D82617#2117441 <https://reviews.llvm.org/D82617#2117441>, @sammccall wrote:
> >
> > > In D82617#2117086 <https://reviews.llvm.org/D82617#2117086>, @Quuxplusone wrote:
> > >
> > > > FWIW, I think the example you gave is **correct** for GCC to warn on.
> > >
> > >
> > > Everything the warning says is correct, but in this pattern the polymorphic usage is the whole point of the hierarchy, the subclasses are never exposed. There's no actual danger of confusion.
> > >
> > > > the derived class violates the Liskov substitution principle: it doesn't have an `obj.foo(42)` method.
> > >
> > > LSP doesn't say the classes are substitutable (indeed you couldn't template over the subclasses, for example). It says that *objects* of the classes should be. And they are.
> >
> >
> > I don't think I agree with this sentiment - "template<typename T> void f(T t); int main() { base b; f(b); }" I think when it says (to quote the English formulation in the Wikipedia article) " if S is a subtype of T, then objects of type T may be replaced with objects of type S (i.e. an object of type T may be substituted with any object of a subtype S) without altering any of the desirable properties of the program"
> >
> > Then replacing "base b" with "derived d" should keep working
>
>
> You haven't just replaced the object type though, you've replaced the variable type too. If the variable type stays the same (say base&) and the object type changes, then we see that `derived`s are valid `base`s, which is what LSP is asking. (Note the examples are things like covariant return types, which fit this pattern)
>
> I'm not sure this matters - the precise definition of a particular piece of jargon doesn't imply we should draw our lines exactly there. But if we're going to cite this authority I'd like to be clear on what it claims :-)


Might continue to disagree there... but even if not that authority, I think it's pretty confusing when a derived class doesn't offer the same API as its public base class due to hiding like this.

>> I think that's a pretty reasonable thing & I think maybe it's OK to live up to GCC's version of this warning.
>>  ...
>>  But I don't mind conforming to GCC's more stylistic indication.
> 
> What's the right outcome for OK/don't mind?
> 
> 1. people can write code with this style in mind? agree, no question
> 2. should continue to enforce for clang proper, absent consensus to change? I can live with that, wouldn't be my least favorite element of clang style ;-)
> 3. should enforce for all clang-related subprojects, even where the consensus is they do mind? I think stronger arguments are needed. (Uniformity would be one, but clang is the only part of llvm with this warning, so I think this actually cuts the other way)
> 
>   Basically, I'm waiting for someone to say "I want this warning on for clang, so just disable it for clangd". The focus on which style is better leaves me pretty unclear on whether people actually care about the warning or just want to air some style opinions.

Guess perhaps a different question: Why don't you want this for clangd? Does it make the codebase better by not adhering to this particular warning? At least at the moment, I don't think it does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82617