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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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

> Abandoning, we'll do this in clangd or find an acceptable way to silence it (see D82736 <https://reviews.llvm.org/D82736>).
>
> In D82617#2119144 <https://reviews.llvm.org/D82617#2119144>, @dblaikie wrote:
>
> > In D82617#2119138 <https://reviews.llvm.org/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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82617