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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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 - I think that's a pretty reasonable thing & I think maybe it's OK to live up to GCC's version of this warning.

I made some improvements to Clang's -Woverloaded-virtual to get it good enough to turn on for LLVM builds a while back (r166254 r166154) & did rather like that Clang's didn't warn in this particular way GCC does that you're talking about - but over time, looking at this particular issue that @Quuxplusone mentions, I kind of see where GCC is coming from. Though from a "Bug finding" perspective, it's probably better to warn at call-sites if "Derived D; D.func(...);" resolved to a different func due to lack of visibility of base functions. But I don't mind conforming to GCC's more stylistic indication.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82617