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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
