Bug 20423 - Warning -Woverloaded-virtual triggers to often
Summary: Warning -Woverloaded-virtual triggers to often
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.0.0
: P2 minor
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic, easyhack, patch
: 20683 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-03-11 10:34 UTC by Michael Cieslinski
Modified: 2015-03-27 15:31 UTC (History)
7 users (show)

See Also:
Host: x86_64-unknown-linux-gnu
Target: x86_64-unknown-linux-gnu
Build: x86_64-unknown-linux-gnu
Known to work:
Known to fail:
Last reconfirmed: 2009-07-17 10:00:52


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Cieslinski 2005-03-11 10:34:19 UTC
I tried the warning option -Woverloaded-virtual in my application 
and it was very usefull to me (I managed to find a serious bug). 

However, the warning triggers triggers too often. 
Consider the following test case:


class Foo
{
public: virtual void Func(int);
        virtual void Func(int, int);
};

class Baz: public Foo
{
public: virtual void Func(int, int);
};


When compiled with gcc40 I get:

warntest.cpp:4: warning: 'virtual void Foo::Func(int)' was hidden
warntest.cpp:10: warning:   by 'virtual void Baz::Func(int, int)'

The warning is triggered only if I try to overload one of the two
Func in Foo. If both Func are declared in Baz no warning is issued.

Since our application it is common to overload only some methods of
a base class I get thousands of these warnings.


This is also not conforming to the "specification" in
http://gcc.gnu.org/ml/gcc-bugs/1999-08n/msg01069.html

  Warn when a derived class function declaration may be an error in
  defining a virtual function.  In a derived class, the definitions of
  virtual functions must match the type signature of a virtual
  function declared in the base class.  With this option, the compiler
  warns when you define a function with the same name as a virtual
  function, but with a type signature that does not match any
  declarations from the base class.
 

By the way: I find this much better understandable than the description
in the gcc manual

`-Woverloaded-virtual (C++ only)'                                   
     Warn when a function declaration hides virtual functions from a
     base class.  For example, in:                                  
                                                                    
          struct A {                                                
            virtual void f();                                       
          };                                                        
                                                                    
          struct B: public A {                                      
            void f(int);                                            
          };                                                        
                                                                    
     the `A' class version of `f' is hidden in `B', and code like:  
                                                                    
          B* b;                                                     
          b->f();                                                   
                                                                    
     will fail to compile.                                          

Michael Cieslinski
Comment 1 Andrew Pinski 2005-03-11 15:11:57 UTC
Hmm, ICC gives:
t.cc(7): warning #654: overloaded virtual function "Foo::Func" is only partially overridden in class "Baz"
  class Baz: public Foo
        ^

t.cc(14): error #165: too few arguments in function call
    b->Func(1);
             ^

compilation aborted for t.cc (code 2)

The warning is correct because Func(int) is still hidden in the supper class and you cannot use it:
class Foo
{
public: virtual void Func(int);
        virtual void Func(int, int);
};

class Baz: public Foo
{
public: virtual void Func(int, int);
};

void g(Baz *b)
{
  b->Func(1);
}


Maybe we should do the warning which ICC is giving instead of the one which we give right now.
Comment 2 Michael Cieslinski 2005-03-11 17:30:00 UTC
I think there a two different situations:

1) in the derived class you define a method with the same name but
   different parameters. 

This is typically a serious bug in your program which can result in the 
exectution of the code of the wrong (not overloaded) function.


2) in the derived class you define not all overloaded variants. 

In that case the not overloaded variants are hidden but no run-time-error 
occurs. If you try to call a hidden variant, the compiler complains.


If "-Woverloaded-virtual" only triggers in case 1, it could be enabled
with "-Wall" as it typically indicates a severe bug in your program.

If there would be a "-Wpartial-overloaded-virtual" you could enable if you like 
it and you can. I have to work with a large existing code basis where I would 
not enable it.

I think to give different messages like ICC is the way to go.

Michael Cieslinski
Comment 3 Andrew Pinski 2005-03-29 14:10:35 UTC
*** Bug 20683 has been marked as a duplicate of this bug. ***
Comment 4 Gabriel Dos Reis 2005-11-24 02:23:45 UTC
Agreed with the reporter's analysis
Comment 5 Manuel López-Ibáñez 2007-02-14 22:30:30 UTC
I am looking at this but it is a bit hard to see how to make the distinction between the two kinds. I don't want to introduce yet another loop in warn_hidden but perhaps that is unavoidable. Any hints?
Comment 6 Manuel López-Ibáñez 2007-02-16 10:01:48 UTC
A really wild-guess patch. Comments?


Index: gcc/cp/class.c
===================================================================
--- gcc/cp/class.c      (revision 121953)
+++ gcc/cp/class.c      (working copy)
@@ -2377,6 +2377,8 @@ warn_hidden (tree t)
       tree binfo;
       int j;

+      bool just_hidden = false;
+
       /* All functions in this slot in the CLASSTYPE_METHOD_VEC will
         have the same name.  Figure out what name that is.  */
       name = DECL_NAME (OVL_CURRENT (fns));
@@ -2408,8 +2410,14 @@ warn_hidden (tree t)
                /* If the method from the base class has the same
                   signature as the method from the derived class, it
                   has been overridden.  */
-               if (same_signature_p (fndecl, TREE_VALUE (*prev)))
-                 *prev = TREE_CHAIN (*prev);
+               if (same_signature_p (fndecl, TREE_VALUE (*prev)))
+                 {
+                   *prev = TREE_CHAIN (*prev);
+                   /* If at least one method has the same signature,
+                      the not overloaded variants are just
+                      hidden.  */
+                   just_hidden = true;
+                 }
                else
                  prev = &TREE_CHAIN (*prev);
            }
@@ -2419,9 +2427,17 @@ warn_hidden (tree t)
         as they are hidden.  */
       while (base_fndecls)
        {
-         /* Here we know it is a hider, and no overrider exists.  */
-         warning (0, "%q+D was hidden", TREE_VALUE (base_fndecls));
-         warning (0, "  by %q+D", fns);
+         /* If Here we know it is a hider, and no overrider exists.  */
+         if (just_hidden)
+           {
+             warning (OPT_Wpartial_overloaded_virtual, "%q+D was hidden", TREE_VALUE (base_fndecls));
+             warning (OPT_Wpartial_overloaded_virtual, "  by %q+D", fns);
+           }                                                                                           
+         else
+           {
+             warning (OPT_Woverloaded_virtual, "%q+D was hidden", TREE_VALUE (base_fndecls));
+             warning (OPT_Woverloaded_virtual, "  by %q+D", fns);
+           }
          base_fndecls = TREE_CHAIN (base_fndecls);
        }
     }
Comment 7 André Wöbbeking 2008-11-30 15:36:37 UTC
Any progress on this? 

This warning could be really useful if only 1) would be handled. In its current state I can't use it as I get too many "false" positives :-(
Comment 8 Jason Merrill 2009-07-18 06:10:29 UTC
An old patch: http://gcc.gnu.org/ml/gcc-patches/1999-03n/msg00182.html
Comment 9 Jonathan Wakely 2009-07-21 17:36:04 UTC
(In reply to comment #0)
> 
> This is also not conforming to the "specification" in
> http://gcc.gnu.org/ml/gcc-bugs/1999-08n/msg01069.html
> 
>   Warn when a derived class function declaration may be an error in
>   defining a virtual function.  In a derived class, the definitions of
>   virtual functions must match the type signature of a virtual
>   function declared in the base class.  With this option, the compiler
>   warns when you define a function with the same name as a virtual
>   function, but with a type signature that does not match any
>   declarations from the base class.

But that's not what it does. The description in the manual describes the current behaviour correctly, that description above doesn't.

(In reply to comment #6)
> A really wild-guess patch. Comments?
...
> +                   /* If at least one method has the same signature,
> +                      the not overloaded variants are just
> +                      hidden.  */
> +                   just_hidden = true;

Shouldn't this say "not overridden variants" ?

I don't like the name -Wpartial-overloaded-virtual for the same reason. The name should clearly distinguish whether it is triggered by overloading, overriding or hiding.

-Woverloaded-virtual is a reasonable name for the current behaviour; it triggers if there is an overload that hides a virtual (whether the overload is itself virtual or not.)  The proposed -Wpartial-overloaded-virtual name makes no sense.
Comment 10 Jonathan Wakely 2009-08-11 14:24:10 UTC
as with bug 31937, there is overlap between this enhancement and the 'Explicit Virtual Function Overrides' paper, http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2009/n2928.htm

the attributes proposed in that paper allow the derived class to explicitly state whether a function should override or hide the same name in the base class, making it ill-formed if there is unintended hiding
Comment 11 Jason Merrill 2010-02-19 21:23:07 UTC
Not working on this now.