[PATCH] -Woverloaded-virtual

[Aaron Ballman]

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
>>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: SemaDeclCXX.cpp.patch
Type: application/octet-stream
Size: 952 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140730/69b1783a/attachment.obj>