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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82617