Disable GCC's -Woverloaded-virtual, which has false positives.

[sammccall]

Summary

Currently this warning is on for both clang and GCC, when building clang.
It's off for the rest of LLVM, so my sense is it isn't that vital.

Clang's version seems to be OK: it warns if you're declaring a method that's
virtual in the base, but not overriding.

GCC's version warns whenever there's name hiding, even if you are overriding
something. It fires on this legitimate pattern:

class Base {
  virtual void foo(); // to be overridden
  void foo(int); // implemented in terms of foo()
};

foo(int) is hidden in derived classes, but foo(int) is used polymorphically
through Base* and works fine there.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=20423

[kadircet]

LGTM, thanks!

This was also my final plan after investigating in https://reviews.llvm.org/D81920, but then I forgot ...

clang/CMakeLists.txt

398

nit: maybe put the gnu bug link in comments

[• Quuxplusone]

FWIW, I think the example you gave is correct for GCC to warn on. You said:

class Base {
  virtual void foo(); // to be overridden
  void foo(int); // implemented in terms of foo()
};

foo(int) is hidden in derived classes. So if someone derives from Base (which someone must, because Base is polymorphic), then the derived class violates the Liskov substitution principle: it doesn't have an obj.foo(42) method. The correct solution is to rename the implementation method from foo() to e.g. fooImpl() or (my personal style, following the NVI idiom) do_foo().

GCC is correct to warn that this code is confusing and hard to maintain because of name hiding. The solution is to adjust the code until it is clear and maintainable, at which point it'll no longer trigger the warning. I don't see any problem with GCC's warning here.

[sammccall]

In 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.

[• Quuxplusone]

In D82617#2117441, @sammccall wrote:

In 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.

If you intend for the method to never be called from a subclass, it should be private in that subclass.
This morning I left a comment on https://reviews.llvm.org/D81920#inline-760189 indicating how I would disentangle that code if I owned it. Hopefully that comment clarifies my position somewhat.

[sammccall]

indicating how I would disentangle that code if I owned it. Hopefully that comment clarifies my position somewhat.

So your position on how to best write that code is interesting (I disagree, but I think there are probably types of projects where your priorities are the right ones. Maybe clang itself is one of them!)

But what would be really useful is to clarify your position on whether the warning in clang is delivering value. E.g. that you do want it enabled for clang, so we should rather turn this off at the clangd level.

[• Quuxplusone]

what would be really useful is to clarify your position on whether the warning in clang is delivering value

I have no special information on that. Clearly someone thought it was adding value when they added it (way back in 2009; https://github.com/llvm/llvm-project/commit/bd1af01fcd07f706a429d980d70437d767837288 ). Have best practices changed in the intervening 11 years so that (GCC's implementation of) -Woverloaded-virtual is no longer a best practice, or no longer adds value? I doubt it.
In this case it's also super cheap to just fix the code. If it took a lot of effort to fix it, or if the fix made the code harder to read, we might have a discussion about whether the cost of keeping -Woverloaded-virtual enabled was worth the negligible benefit; but in this case it seems to me that the cost is zero, so it doesn't matter that its benefit is negligible.

Is this warning catching bugs? We have no way to know, because this warning never fires on the code that people are committing (or at least it doesn't fire for long before they go fix the code). In this specific case I believe it caught an instance of confusing code which can be improved, but which was not literally incorrect (yet).

I see from https://bugs.freedesktop.org/show_bug.cgi?id=91645 that "The intent is to have this warning catch cases where the subclass overrides a superclass method, but then the signature of the superclass is changed and one forgets to update the subclass method, changing the [override into an overload]." This implies to me that if our codebase uses C++11 override correctly everywhere, and enables some (hypothetical) warning that will warn on any place the override keyword seems to be "missing," then maybe... Well, no, even then,
https://godbolt.org/z/xzX8QC
I think it's just flat-out better to follow the warning's advice and avoid name hiding. Keep things simple! The interaction between overloading and overriding is complex and confusing and therefore shouldn't appear in good code. And again, writing simple code is free. Small benefit, but zero cost => good practice.

[dblaikie]

In D82617#2117441, @sammccall wrote:

In 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.

[sammccall]

In D82617#2118118, @dblaikie wrote:

In D82617#2117441, @sammccall wrote:

In 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 deriveds are valid bases, 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 :-)

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.

[dblaikie]

In D82617#2118253, @sammccall wrote:

In D82617#2118118, @dblaikie wrote:

In D82617#2117441, @sammccall wrote:

In 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 deriveds are valid bases, 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.

[sammccall]

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?

Yes, exactly. (Sorry if this wasn't explicit).

[dblaikie]

In D82617#2119138, @sammccall wrote:

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?

Yes, exactly. (Sorry if this wasn't explicit).

Sorry - poor phrasing on my part. Seems we disagree on this - I think it's probably a good thing to adhere to, you don't. I'd like to better understand the difference of opinions.

My take on it is that having derived classes with hidden overloads seems to me like it could result in confusion when using those derived classes (or worse - if the overload set could result in successful compilation even when the desired overload isn't present - possibly bugs, rather than only confusing compilation errors). Seems good to me to keep the derived classes interfaces not containing such surprises.

[sammccall]

Abandoning, we'll do this in clangd or find an acceptable way to silence it (see D82736).

In D82617#2119144, @dblaikie wrote:

In D82617#2119138, @sammccall wrote:

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?

Yes, exactly. (Sorry if this wasn't explicit).

Sorry - poor phrasing on my part. Seems we disagree on this - I think it's probably a good thing to adhere to, you don't.

Yeah, I think we disagree, and that's OK! We need to stop emitting a warning, and apart from that, this doesn't seem like a terribly important question that needs an LLVM-wide policy.

On stylistic questions like this, I think code owners generally decide, right? So my intent was to drop this proposed change for clang (no consensus) and take these opinions under advisement for clangd.

I didn't re-engage here on the substance as I think all the arguments are played out and nobody's mind is being changed, and I figured it was time to move on.

I'd like to better understand the difference of opinions.

Sure, and apologies I didn't read it that way. I can summarize the counterargument as:

1. I agree that this warning flags cases with confusing code, though in some cases (like ThreadsafeFS::view) I think the potential for confusion *due to the name-hiding + overloading pattern* is very low.
2. Marking one method of an overload set virtual and implementing others in terms of it is the most direct way to express what we're trying to do here. (e.g. in Java which has no equivalent name-hiding pattern, nobody really takes issue with this). Therefore if the name-hiding is harmless or close-to-it, it's the clearest expression of this pattern.
3. The question of whether people like the concrete names or the overload set chosen in ThreadsafeFS is irrelevant here.

I think there's a fair amount of disagreement about (1), nobody really presented a counterargument to (2), and for some reason we spent a lot of time on (3) which seems to be a total bikeshed on a tangentially related topic that had been settled in the usual way.

[dblaikie]

In D82617#2119394, @sammccall wrote:

Abandoning, we'll do this in clangd or find an acceptable way to silence it (see D82736).

In D82617#2119144, @dblaikie wrote:

In D82617#2119138, @sammccall wrote:

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?

Yes, exactly. (Sorry if this wasn't explicit).

Sorry - poor phrasing on my part. Seems we disagree on this - I think it's probably a good thing to adhere to, you don't.

Yeah, I think we disagree, and that's OK! We need to stop emitting a warning, and apart from that, this doesn't seem like a terribly important question that needs an LLVM-wide policy.

On stylistic questions like this, I think code owners generally decide, right? So my intent was to drop this proposed change for clang (no consensus) and take these opinions under advisement for clangd.

I didn't re-engage here on the substance as I think all the arguments are played out and nobody's mind is being changed, and I figured it was time to move on.

I'd like to better understand the difference of opinions.

Sure, and apologies I didn't read it that way. I can summarize the counterargument as:

1. I agree that this warning flags cases with confusing code, though in some cases (like ThreadsafeFS::view) I think the potential for confusion *due to the name-hiding + overloading pattern* is very low.

Fair - I'd be willing to classify this particular case as pretty close to a false positive. But for me it'd be under the bucket of false positive's like GCC's -Wparetheses which warns on assert("message" && x || y) (GCC warns about the user probably meaning (x || y), Clang doesn't because it notices you get the same answer with or without the parens) - a bit annoying, but probably harmless enough to add the parens to let GCC users keep getting the warnings (& in that case - just means they'll be less likely to break LLVM self-hosting buildbots than if we disabled the GCC warning entirely)

2. Marking one method of an overload set virtual and implementing others in terms of it is the most direct way to express what we're trying to do here. (e.g. in Java which has no equivalent name-hiding pattern, nobody really takes issue with this). Therefore if the name-hiding is harmless or close-to-it, it's the clearest expression of this pattern.

If there wasn't any name hiding, I wouldn't have a problem with the code as-is. For me it's about the call-sites rather than the implementation. Looking at a derived class never gives you the whole picture of the class's API anyway.

3. The question of whether people like the concrete names or the overload set chosen in ThreadsafeFS is irrelevant here.

Yep - agreed. That's a conversation for other reviews/threads.

I think there's a fair amount of disagreement about (1), nobody really presented a counterargument to (2), and for some reason we spent a lot of time on (3) which seems to be a total bikeshed on a tangentially related topic that had been settled in the usual way.

Appreciate the summary/thoughts!

clang/CMakeLists.txt

 
 # Configure the Version.inc file.
 configure_file(
   ${CMAKE_CURRENT_SOURCE_DIR}/include/clang/Basic/Version.inc.in
   ${CMAKE_CURRENT_BINARY_DIR}/include/clang/Basic/Version.inc)
 
 # Add appropriate flags for GCC
 if (LLVM_COMPILER_IS_GCC_COMPATIBLE)
-  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-common -Woverloaded-virtual")
-  if (NOT "${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-common")
+  if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+    # Clang's version of -Woverloaded-virtual is OK, GCC's is too noisy.
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Woverloaded-virtual")
+  else()
     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-strict-aliasing")
   endif ()
 
   # Enable -pedantic for Clang even if it's not enabled for LLVM.
   if (NOT LLVM_ENABLE_PEDANTIC)
     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -pedantic -Wno-long-long")
   endif ()