[PATCH 1/2] [C++] Remove tree_list from warn_hidden

[PATCH 1/2] [C++] Remove tree_list from warn_hidden

[tsaunders]

From: Trevor Saunders <tsaunders@mozilla.com>

Hi,

I'll claim this is kind of a bug fix in so much as excessive gc allocation and
use of tree_list is a bug.

bootstrapped + regtested x86_64-unknown-linux-gnu, ok?

Trev


cp/

	* class.c (warn_hidden): Use auto_vec<tree> instead of tree_list to
	hold base_fndecls.
	(get_basefndecls): Adjust.


diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index c83c8ad..16279df 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -129,7 +129,7 @@ vec<tree, va_gc> *local_classes;
 static tree get_vfield_name (tree);
 static void finish_struct_anon (tree);
 static tree get_vtable_name (tree);
-static tree get_basefndecls (tree, tree);
+static void get_basefndecls (tree, tree, vec<tree> *);
 static int build_primary_vtable (tree, tree);
 static int build_secondary_vtable (tree);
 static void finish_vtbls (tree);
@@ -2717,16 +2717,16 @@ modify_all_vtables (tree t, tree virtuals)
 /* Get the base virtual function declarations in T that have the
    indicated NAME.  */
 
-static tree
-get_basefndecls (tree name, tree t)
+static void
+get_basefndecls (tree name, tree t, vec<tree> *base_fndecls)
 {
   tree methods;
-  tree base_fndecls = NULL_TREE;
   int n_baseclasses = BINFO_N_BASE_BINFOS (TYPE_BINFO (t));
   int i;
 
   /* Find virtual functions in T with the indicated NAME.  */
   i = lookup_fnfields_1 (t, name);
+  bool found_decls = false;
   if (i != -1)
     for (methods = (*CLASSTYPE_METHOD_VEC (t))[i];
 	 methods;
@@ -2736,20 +2736,20 @@ get_basefndecls (tree name, tree t)
 
 	if (TREE_CODE (method) == FUNCTION_DECL
 	    && DECL_VINDEX (method))
-	  base_fndecls = tree_cons (NULL_TREE, method, base_fndecls);
+	  {
+	    base_fndecls->safe_push (method);
+	    found_decls = true;
+	  }
       }
 
-  if (base_fndecls)
-    return base_fndecls;
+  if (found_decls)
+    return;
 
   for (i = 0; i < n_baseclasses; i++)
     {
       tree basetype = BINFO_TYPE (BINFO_BASE_BINFO (TYPE_BINFO (t), i));
-      base_fndecls = chainon (get_basefndecls (name, basetype),
-			      base_fndecls);
+      get_basefndecls (name, basetype, base_fndecls);
     }
-
-  return base_fndecls;
 }
 
 /* If this declaration supersedes the declaration of
@@ -2811,7 +2811,6 @@ warn_hidden (tree t)
       tree fn;
       tree name;
       tree fndecl;
-      tree base_fndecls;
       tree base_binfo;
       tree binfo;
       int j;
@@ -2820,19 +2819,18 @@ warn_hidden (tree t)
 	 have the same name.  Figure out what name that is.  */
       name = DECL_NAME (OVL_CURRENT (fns));
       /* There are no possibly hidden functions yet.  */
-      base_fndecls = NULL_TREE;
+      auto_vec<tree, 20> base_fndecls;
       /* Iterate through all of the base classes looking for possibly
 	 hidden functions.  */
       for (binfo = TYPE_BINFO (t), j = 0;
 	   BINFO_BASE_ITERATE (binfo, j, base_binfo); j++)
 	{
 	  tree basetype = BINFO_TYPE (base_binfo);
-	  base_fndecls = chainon (get_basefndecls (name, basetype),
-				  base_fndecls);
+	  get_basefndecls (name, basetype, &base_fndecls);
 	}
 
       /* If there are no functions to hide, continue.  */
-      if (!base_fndecls)
+      if (!base_fndecls.exists ())
 	continue;
 
       /* Remove any overridden functions.  */
@@ -2842,28 +2840,27 @@ warn_hidden (tree t)
 	  if (TREE_CODE (fndecl) == FUNCTION_DECL
 	      && DECL_VINDEX (fndecl))
 	    {
-	      tree *prev = &base_fndecls;
-
-	      while (*prev)
 		/* 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);
-		else
-		  prev = &TREE_CHAIN (*prev);
+		for (size_t k = 0; k < base_fndecls.length (); k++)
+		if (base_fndecls[k]
+		    && same_signature_p (fndecl, base_fndecls[k]))
+		  base_fndecls[k] = NULL_TREE;
 	    }
 	}
 
       /* Now give a warning for all base functions without overriders,
 	 as they are hidden.  */
-      while (base_fndecls)
-	{
-	  /* Here we know it is a hider, and no overrider exists.  */
-	  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);
-	}
+      size_t k;
+      tree base_fndecl;
+      FOR_EACH_VEC_ELT (base_fndecls, k, base_fndecl)
+	if (base_fndecl)
+	  {
+	      /* Here we know it is a hider, and no overrider exists.  */
+	      warning (OPT_Woverloaded_virtual, "%q+D was hidden", base_fndecl);
+	      warning (OPT_Woverloaded_virtual, "  by %q+D", fns);
+	  }
     }
 }
 
-- 
2.1.3

[PATCH 2/2] [C++] pr31397 - implement -Wsuggest-override

[tsaunders]

From: Trevor Saunders <tsaunders@mozilla.com>

Hi,

the idea in the pr of providing a warning that tells people where they can add
override seems reasonable to me.

bootstrapped + regtested x86_64-unknown--linux-gnu (new test passes), ok?

Trev

c-family/

	* c.opt (Wsuggest-override): New option.

cp/

	* class.c (check_for_override): Warn when a virtual function is an
	override not marked override.

gcc/

	* doc/invoke.texi: Document -Wsuggest-override.


diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 85dcb98..259b520 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -574,6 +574,11 @@ Wsuggest-attribute=format
 C ObjC C++ ObjC++ Var(warn_suggest_attribute_format) Warning
 Warn about functions which might be candidates for format attributes
 
+Wsuggest-override
+C++ ObjC++ Var(warn_override) Warning
+Suggest that the override keyword be used when the declaration of a virtual
+function overrides another.
+
 Wswitch
 C ObjC C++ ObjC++ Var(warn_switch) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
 Warn about enumerated switches, with no default, missing a case
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 16279df..515f33f 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -2777,6 +2777,10 @@ check_for_override (tree decl, tree ctype)
     {
       DECL_VINDEX (decl) = decl;
       overrides_found = true;
+      if (warn_override && DECL_VIRTUAL_P (decl) && !DECL_OVERRIDE_P (decl)
+	  && !DECL_DESTRUCTOR_P (decl))
+	warning_at(DECL_SOURCE_LOCATION (decl), OPT_Wsuggest_override,
+		   "%q+D can be marked override", decl);
     }
 
   if (DECL_VIRTUAL_P (decl))
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 89edddb..8741e8e 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -275,7 +275,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wstack-protector -Wstack-usage=@var{len} -Wstrict-aliasing @gol
 -Wstrict-aliasing=n @gol -Wstrict-overflow -Wstrict-overflow=@var{n} @gol
 -Wsuggest-attribute=@r{[}pure@r{|}const@r{|}noreturn@r{|}format@r{]} @gol
--Wsuggest-final-types @gol -Wsuggest-final-methods @gol
+-Wsuggest-final-types @gol -Wsuggest-final-methods @gol -Wsuggest-override @gol
 -Wmissing-format-attribute @gol
 -Wswitch  -Wswitch-default  -Wswitch-enum -Wswitch-bool -Wsync-nand @gol
 -Wsystem-headers  -Wtrampolines  -Wtrigraphs  -Wtype-limits  -Wundef @gol
@@ -4255,6 +4255,10 @@ effective with link time optimization, where the information about the class
 hiearchy graph is more complete. It is recommended to first consider suggestins
 of @option{-Wsuggest-final-types} and then rebuild with new annotations.
 
+@item -Wsuggest-override
+Warn about overriding virtual functions that are not marked with the override
+keyword.
+
 @item -Warray-bounds
 @opindex Wno-array-bounds
 @opindex Warray-bounds
diff --git a/gcc/testsuite/g++.dg/warn/Wsuggest-override.C b/gcc/testsuite/g++.dg/warn/Wsuggest-override.C
new file mode 100644
index 0000000..929d365
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wsuggest-override.C
@@ -0,0 +1,21 @@
+// { dg-do compile }
+// { dg-options "-std=c++11 -Wsuggest-override" }
+struct A
+{
+	A();
+	virtual ~A();
+	virtual void f();
+	virtual int bar();
+	operator int();
+	virtual operator float();
+};
+
+struct B : A
+{
+	B();
+	virtual ~B();
+	virtual void f(); // { dg-warning "can be marked override" }
+virtual int bar() override;
+operator int();
+virtual operator float(); // { dg-warning "can be marked override" }
+};
-- 
2.1.3

Re: [PATCH 1/2] [C++] Remove tree_list from warn_hidden

[Jason Merrill]

On 11/27/2014 01:28 AM, tsaunders@mozilla.com wrote:
> -      if (!base_fndecls)
> +      if (!base_fndecls.exists ())

This should use is_empty rather than exists.  OK with that change.

Jason

Re: [PATCH 2/2] [C++] pr31397 - implement -Wsuggest-override

[Jason Merrill]

On 11/27/2014 01:28 AM, tsaunders@mozilla.com wrote:
> +      if (warn_override && DECL_VIRTUAL_P (decl) && !DECL_OVERRIDE_P (decl)

Why check DECL_VIRTUAL_P here?

> +	warning_at(DECL_SOURCE_LOCATION (decl), OPT_Wsuggest_override,

Space before the (.

Jason

Re: [PATCH 2/2] [C++] pr31397 - implement -Wsuggest-override

[Trevor Saunders]

On Wed, Dec 17, 2014 at 04:21:31PM -0500, Jason Merrill wrote:
> On 11/27/2014 01:28 AM, tsaunders@mozilla.com wrote:
> >+      if (warn_override && DECL_VIRTUAL_P (decl) && !DECL_OVERRIDE_P (decl)
> 
> Why check DECL_VIRTUAL_P here?

I think it was to avoid warning when decl is hiding a non virtual method
on the parent class, but I guess that won't happen anyway? so I'll
remove it.

Trev

> 
> >+	warning_at(DECL_SOURCE_LOCATION (decl), OPT_Wsuggest_override,
> 
> Space before the (.
> 
> Jason
> 

[PATCH] pr31397 - implement -Wsuggest-override

[tbsaunde+gcc]

From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org>

Hi,

comments fixed.  bootstrapped on x86_64-linux, new test passes and regtest
pending, ok?

Trev


c-family/

	* c.opt (Wsuggest-override): New option.

cp/

	* class.c (check_for_override): Warn when a virtual function is an
	override not marked override.

gcc/

	* doc/invoke.texi: Document -Wsuggest-override.

---
 gcc/c-family/c.opt                            |  5 +++++
 gcc/cp/class.c                                |  4 ++++
 gcc/doc/invoke.texi                           |  6 +++++-
 gcc/testsuite/g++.dg/warn/Wsuggest-override.C | 23 +++++++++++++++++++++++
 4 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wsuggest-override.C

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index f86718b..1f6f793 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -578,6 +578,11 @@ Wsuggest-attribute=format
 C ObjC C++ ObjC++ Var(warn_suggest_attribute_format) Warning
 Warn about functions which might be candidates for format attributes
 
+Wsuggest-override
+C++ ObjC++ Var(warn_override) Warning
+Suggest that the override keyword be used when the declaration of a virtual
+function overrides another.
+
 Wswitch
 C ObjC C++ ObjC++ Var(warn_switch) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
 Warn about enumerated switches, with no default, missing a case
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 0ac2124..d2cf4a0 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -2811,6 +2811,10 @@ check_for_override (tree decl, tree ctype)
     {
       DECL_VINDEX (decl) = decl;
       overrides_found = true;
+      if (warn_override && !DECL_OVERRIDE_P (decl)
+	  && !DECL_DESTRUCTOR_P (decl))
+	warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wsuggest_override,
+		    "%q+D can be marked override", decl);
     }
 
   if (DECL_VIRTUAL_P (decl))
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 9f56f42..d0ee093 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -277,7 +277,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wstack-protector -Wstack-usage=@var{len} -Wstrict-aliasing @gol
 -Wstrict-aliasing=n @gol -Wstrict-overflow -Wstrict-overflow=@var{n} @gol
 -Wsuggest-attribute=@r{[}pure@r{|}const@r{|}noreturn@r{|}format@r{]} @gol
--Wsuggest-final-types @gol -Wsuggest-final-methods @gol
+-Wsuggest-final-types @gol -Wsuggest-final-methods @gol -Wsuggest-override @gol
 -Wmissing-format-attribute @gol
 -Wswitch  -Wswitch-default  -Wswitch-enum -Wswitch-bool -Wsync-nand @gol
 -Wsystem-headers  -Wtrampolines  -Wtrigraphs  -Wtype-limits  -Wundef @gol
@@ -4276,6 +4276,10 @@ class hierarchy graph is more complete. It is recommended to first consider
 suggestions of @option{-Wsuggest-final-types} and then rebuild with new
 annotations.
 
+@item -Wsuggest-override
+Warn about overriding virtual functions that are not marked with the override
+keyword.
+
 @item -Warray-bounds
 @opindex Wno-array-bounds
 @opindex Warray-bounds
diff --git a/gcc/testsuite/g++.dg/warn/Wsuggest-override.C b/gcc/testsuite/g++.dg/warn/Wsuggest-override.C
new file mode 100644
index 0000000..f820f4b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wsuggest-override.C
@@ -0,0 +1,23 @@
+// { dg-do compile }
+// { dg-options "-std=c++11 -Wsuggest-override" }
+struct A
+{
+	A();
+	virtual ~A();
+	virtual void f();
+	virtual int bar();
+	int c();
+	operator int();
+	virtual operator float();
+};
+
+struct B : A
+{
+	B();
+	virtual ~B();
+	virtual void f(); // { dg-warning "can be marked override" }
+virtual int bar() override;
+int c();
+operator int();
+virtual operator float(); // { dg-warning "can be marked override" }
+};
-- 
2.1.3

Re: [PATCH] pr31397 - implement -Wsuggest-override

[Jason Merrill]

OK.

Jason