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