[PATCH] -Woverloaded-virtual

[David Blaikie]

I think something pithier might be helpful (while your comment is
correct, I found it a little hard to follow the motivations, etc) -
spitballing:

Here Clang deviates from GCC by only diagnosing overloads of inherited
virtual function that do not override any other virtual functions in
the base.
GCC's -Woverloaded-virtual is more about any case of derived functions
hiding base functions and doesn't seem specific to virtual functions.
The cases GCC catches that Clang does not may be better served by a
general warning on call sites that, had the base function been
visible, would've called that function. This would avoid false
positives such as warning on base::func() + derived::func(int) where
callers are unlikely to call the latter when they intended the former
(and Clang's typo correction can correct derived::func() to
base::func() already).

Maybe that's too much of a rant. I'm not sure how much detail to give,
really... The last sentence there, while useful, is perhaps too much
detail - maybe it'd be better served by an example in a test case
somewhere with a FIXME of "warn here"? (and/or, as you say, a bug
filed)

On Wed, Jul 30, 2014 at 3:02 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> After discussion in IRC, David convinced me that this really should be
> a call site diagnostic instead of using this approach. So instead,
> here is a patch that adds some comments explaining the rationale.
> David, do you think I've captured it properly?
>
> ~Aaron
>
> On Wed, Jul 30, 2014 at 5:30 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>> On Wed, Jul 30, 2014 at 4:42 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>> I made some improvements to -Woverloaded-virtual a while ago and
>>> semi-deliberately chose the behavior you're observing (in the sense
>>> that it seemed like this warning was for catching the case where you
>>> tried to override a virtual function but accidentally ended up
>>> overloading it).
>>
>> That's good to know, I kind of wondered whether this behavior was
>> deliberate or not.
>>
>>> The situation you've described might be better suited to a separate
>>> flag
>>
>> How much do we care about diverging from GCC in this regard?
>>
>>> - and should warn even if there's no overriding going on. This
>>> could fire on non-virtual functions too:
>>>
>>> struct base {
>>>   void func(char);
>>> };
>>>
>>> struct derived: base {
>>>   void func(int);
>>> };
>>>
>>> We currently don't warn here.
>>
>> I could see this making some sense under the same flag as the virtual
>> case. In either case, it's a hiding issue that prevents overloading.
>>
>>> Though I've done this deliberately in one or two places (where I
>>> didn't care about the base function, but had a derived function with
>>> the same naem & different args) - the right warning's probably to warn
>>> at the call site and tell people "this call site would've called a
>>> different function in the base class, if it were visible".
>>
>> That seems like a far more tricky diagnostic though (due to converting
>> constructors, conversion operators, etc); it would likely have higher
>> false positive rates, would it not?
>>
>> ~Aaron
>>
>>>
>>> On Wed, Jul 30, 2014 at 1:30 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>>> I believe the following patch addresses a slight deficiency in our
>>>> -Woverloaded-virtual warning check. Specifically, it causes the
>>>> following code to warn (which matches GCC's behavior):
>>>>
>>>> struct base {
>>>>   virtual void foo(int I) {}
>>>>   virtual void foo(char C) {}
>>>> };
>>>>
>>>> struct derived : public base {
>>>>   void foo(int I2) override {}
>>>> };
>>>>
>>>> It does this by continuing to check other methods instead of early
>>>> returning out of processing them when an exact signature match is
>>>> encountered. I believe this is an improvement because it catches
>>>> problems like:
>>>>
>>>> derived d;
>>>> d.foo('1');
>>>>
>>>> Where derived::foo(int) is called when the user may expect
>>>> base::foo(char) to be called.
>>>>
>>>> ~Aaron
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>