[PATCH] c++: Additional warning for name-hiding [PR12341]

[priour.be@gmail.com]

From: benjamin priour <vultkayn@gcc.gnu.org>

Hi,

This patch was the first I wrote and had been
at that time returned to me because ill-formatted.

Getting busy with other things, I forgot about it.
I've now fixed the formatting.

Succesfully regstrapped on x86_64-linux-gnu off trunk
a7d052b3200c7928d903a0242b8cfd75d131e374.
Is it OK for trunk ?

Thanks,
Benjamin.

Patch below.
---

Add a new warning for name-hiding. When a class's field
is named similarly to one inherited, a warning should
be issued.
This new warning is controlled by the existing Wshadow.

gcc/cp/ChangeLog:

	PR c++/12341
	* search.cc (lookup_member):
	New optional parameter to preempt processing the
	inheritance tree deeper than necessary.
	(lookup_field): Likewise.
	(dfs_walk_all): Likewise.
	* cp-tree.h: Update the above declarations.
	* class.cc: (warn_name_hiding): New function.
	(finish_struct_1): Call warn_name_hiding if -Wshadow.

gcc/testsuite/ChangeLog:

	PR c++/12341
	* g++.dg/pr12341-1.C: New file.
	* g++.dg/pr12341-2.C: New file.

Signed-off-by: benjamin priour <vultkayn@gcc.gnu.org>
---
 gcc/cp/class.cc                  | 75 ++++++++++++++++++++++++++++++++
 gcc/cp/cp-tree.h                 |  9 ++--
 gcc/cp/search.cc                 | 28 ++++++++----
 gcc/testsuite/g++.dg/pr12341-1.C | 65 +++++++++++++++++++++++++++
 gcc/testsuite/g++.dg/pr12341-2.C | 34 +++++++++++++++
 5 files changed, 200 insertions(+), 11 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr12341-1.C
 create mode 100644 gcc/testsuite/g++.dg/pr12341-2.C

diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index 778759237dc..b1c59c392a0 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -3080,6 +3080,79 @@ warn_hidden (tree t)
       }
 }
 
+/* Warn about non-static fields name hiding.  */
+
+static void
+warn_name_hiding (tree t)
+{
+  if (is_empty_class (t) || CLASSTYPE_NEARLY_EMPTY_P (t))
+    return;
+
+  for (tree field = TYPE_FIELDS (t); field; field = DECL_CHAIN (field))
+    {
+      /* Skip if field is not an user-defined non-static data member.  */
+      if (TREE_CODE (field) != FIELD_DECL || DECL_ARTIFICIAL (field))
+	continue;
+
+      unsigned j;
+      tree name = DECL_NAME (field);
+      /* Skip if field is anonymous.  */
+      if (!name || !identifier_p (name))
+	continue;
+
+      auto_vec<tree> base_vardecls;
+      tree binfo;
+      tree base_binfo;
+      /* Iterate through all of the base classes looking for possibly
+	 shadowed non-static data members.  */
+      for (binfo = TYPE_BINFO (t), j = 0;
+	   BINFO_BASE_ITERATE (binfo, j, base_binfo); j++)
+	{
+	  tree basetype = BINFO_TYPE (base_binfo);
+	  tree candidate = lookup_field (basetype, name,
+					 /* protect */ 2,
+					 /* want_type */ 0,
+					 /* once_suffices */ true);
+	  if (candidate)
+	    {
+	      /* If we went up the hierarchy to a base class with multiple
+		 inheritance, there could be multiple matches in which case
+		 a TREE_LIST is returned.  */
+	      if (TREE_TYPE (candidate) == error_mark_node)
+		{
+		  for (; candidate; candidate = TREE_CHAIN (candidate))
+		    {
+		      tree candidate_field = TREE_VALUE (candidate);
+		      tree candidate_klass = DECL_CONTEXT (candidate_field);
+		      if (accessible_base_p (t, candidate_klass, true))
+			base_vardecls.safe_push (candidate_field);
+		    }
+		}
+	      else if (accessible_base_p (t, DECL_CONTEXT (candidate), true))
+		base_vardecls.safe_push (candidate);
+	    }
+	}
+
+      /* Field was not found among the base classes.  */
+      if (base_vardecls.is_empty ())
+	continue;
+
+      /* Emit a warning for each field similarly named found
+	 in the base class hierarchy.  */
+      for (tree base_vardecl : base_vardecls)
+	{
+	  if (base_vardecl)
+	    {
+	      auto_diagnostic_group d;
+	      if (warning_at (location_of (field), OPT_Wshadow,
+			      "%qD might shadow %qD", field, base_vardecl))
+		inform (location_of (base_vardecl),
+			"  %qD name already in use here", base_vardecl);
+	    }
+	}
+    }
+}
+
 /* Recursive helper for finish_struct_anon.  */
 
 static void
@@ -7654,6 +7727,8 @@ finish_struct_1 (tree t)
 
   if (warn_overloaded_virtual)
     warn_hidden (t);
+  if (warn_shadow)
+    warn_name_hiding (t);
 
   /* Class layout, assignment of virtual table slots, etc., is now
      complete.  Give the back end a chance to tweak the visibility of
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 3ca011c61c8..890326f0fd8 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7554,11 +7554,13 @@ extern tree lookup_base                         (tree, tree, base_access,
 extern tree dcast_base_hint			(tree, tree);
 extern int accessible_p				(tree, tree, bool);
 extern int accessible_in_template_p		(tree, tree);
-extern tree lookup_field			(tree, tree, int, bool);
+extern tree lookup_field			(tree, tree, int, bool,
+						 bool once_suffices = false);
 extern tree lookup_fnfields			(tree, tree, int, tsubst_flags_t);
 extern tree lookup_member			(tree, tree, int, bool,
 						 tsubst_flags_t,
-						 access_failure_info *afi = NULL);
+						 access_failure_info *afi = NULL,
+						 bool once_suffices = false);
 extern tree lookup_member_fuzzy			(tree, tree, bool);
 extern tree locate_field_accessor		(tree, tree, bool);
 extern int look_for_overrides			(tree, tree);
@@ -7577,7 +7579,8 @@ extern tree binfo_for_vbase			(tree, tree);
 extern tree look_for_overrides_here		(tree, tree);
 #define dfs_skip_bases ((tree)1)
 extern tree dfs_walk_all (tree, tree (*) (tree, void *),
-			  tree (*) (tree, void *), void *);
+			  tree (*) (tree, void *), void *,
+			  bool stop_on_success = false);
 extern tree dfs_walk_once (tree, tree (*) (tree, void *),
 			   tree (*) (tree, void *), void *);
 extern tree binfo_via_virtual			(tree, tree);
diff --git a/gcc/cp/search.cc b/gcc/cp/search.cc
index cd80f285ac9..32bd35f3744 100644
--- a/gcc/cp/search.cc
+++ b/gcc/cp/search.cc
@@ -1145,13 +1145,19 @@ build_baselink (tree binfo, tree access_binfo, tree functions, tree optype)
 
    WANT_TYPE is 1 when we should only return TYPE_DECLs.
 
+   ONCE_SUFFICES is 1 when we should return upon first find of
+   the member in a branch of the inheritance hierarchy tree, rather
+   than collect all similarly named members further in that branch.
+   Does not impede other parallel branches of the tree.
+
    If nothing can be found return NULL_TREE and do not issue an error.
 
    If non-NULL, failure information is written back to AFI.  */
 
 tree
 lookup_member (tree xbasetype, tree name, int protect, bool want_type,
-	       tsubst_flags_t complain, access_failure_info *afi /* = NULL */)
+	       tsubst_flags_t complain, access_failure_info *afi /* = NULL */,
+	       bool once_suffices /* = false */)
 {
   tree rval, rval_binfo = NULL_TREE;
   tree type = NULL_TREE, basetype_path = NULL_TREE;
@@ -1202,7 +1208,7 @@ lookup_member (tree xbasetype, tree name, int protect, bool want_type,
   lfi.type = type;
   lfi.name = name;
   lfi.want_type = want_type;
-  dfs_walk_all (basetype_path, &lookup_field_r, NULL, &lfi);
+  dfs_walk_all (basetype_path, &lookup_field_r, NULL, &lfi, once_suffices);
   rval = lfi.rval;
   rval_binfo = lfi.rval_binfo;
   if (rval_binfo)
@@ -1392,10 +1398,12 @@ lookup_member_fuzzy (tree xbasetype, tree name, bool want_type_p)
    return NULL_TREE.  */
 
 tree
-lookup_field (tree xbasetype, tree name, int protect, bool want_type)
+lookup_field (tree xbasetype, tree name, int protect, bool want_type,
+	      bool once_suffices /* = false */)
 {
   tree rval = lookup_member (xbasetype, name, protect, want_type,
-			     tf_warning_or_error);
+			     tf_warning_or_error, /* afi */ NULL,
+			     once_suffices);
 
   /* Ignore functions, but propagate the ambiguity list.  */
   if (!error_operand_p (rval)
@@ -1480,11 +1488,14 @@ adjust_result_of_qualified_name_lookup (tree decl,
    of PRE_FN and POST_FN can be NULL.  At each node, PRE_FN and
    POST_FN are passed the binfo to examine and the caller's DATA
    value.  All paths are walked, thus virtual and morally virtual
-   binfos can be multiply walked.  */
+   binfos can be multiply walked.
+   If STOP_ON_SUCCESS is 1, do not walk deeper the current hierarchy
+   tree once PRE_FN returns non-NULL.  */
 
 tree
 dfs_walk_all (tree binfo, tree (*pre_fn) (tree, void *),
-	      tree (*post_fn) (tree, void *), void *data)
+	      tree (*post_fn) (tree, void *), void *data,
+	      bool stop_on_success /* = false */)
 {
   tree rval;
   unsigned ix;
@@ -1496,7 +1507,7 @@ dfs_walk_all (tree binfo, tree (*pre_fn) (tree, void *),
       rval = pre_fn (binfo, data);
       if (rval)
 	{
-	  if (rval == dfs_skip_bases)
+	  if (rval == dfs_skip_bases || stop_on_success)
 	    goto skip_bases;
 	  return rval;
 	}
@@ -1505,7 +1516,8 @@ dfs_walk_all (tree binfo, tree (*pre_fn) (tree, void *),
   /* Find the next child binfo to walk.  */
   for (ix = 0; BINFO_BASE_ITERATE (binfo, ix, base_binfo); ix++)
     {
-      rval = dfs_walk_all (base_binfo, pre_fn, post_fn, data);
+      rval = dfs_walk_all (base_binfo, pre_fn, post_fn,
+			   data, stop_on_success);
       if (rval)
 	return rval;
     }
diff --git a/gcc/testsuite/g++.dg/pr12341-1.C b/gcc/testsuite/g++.dg/pr12341-1.C
new file mode 100644
index 00000000000..d66b77a921d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr12341-1.C
@@ -0,0 +1,65 @@
+// PR c++/12341
+/* { dg-do compile } */
+/* { dg-additional-options "-Wshadow" } */
+
+class A {
+protected:
+  int aaa; /* { dg-line A_def_aaa } */
+};
+
+// Check simple inheritance is checked for.
+class B : public A {
+public:
+  int aaa; /* { dg-line B_shadowing_aaa } */
+  /* { dg-warning "'B::aaa' might shadow 'A::aaa'" "" { target *-*-* } .-1 } */
+  /* { dg-note "'A::aaa' name already in use here" "" { target *-*-* } A_def_aaa } */
+private:
+  int bbb; /* { dg-line B_def_bbb } */
+  int eee; /* { dg-line B_def_eee } */
+};
+
+// Check that visibility of base classes's fields is of no concern.
+class C : public B {
+public:
+  int bbb; /* { dg-warning "'C::bbb' might shadow 'B::bbb'" } */
+  /* { dg-note "'B::bbb' name already in use here" "" { target *-*-* } B_def_bbb } */
+};
+
+class D {
+protected:
+  int bbb; /* { dg-line D_def_bbb } */
+  int ddd; /* { dg-line D_def_ddd } */
+};
+
+class E : protected D {
+private:
+  int eee; /* { dg-line E_def_eee } */
+};
+
+// all first-level base classes must be considered.
+class Bi : protected B, public E {
+protected:
+  /* Check that we stop on first ambiguous match,
+  that we don't walk the hierarchy deeper. */
+  int aaa; /* { dg_bogus "'Bi::aaa' might shadow 'A::aaa'" } */
+  /* { dg-warning "'Bi::aaa' might shadow 'B::aaa'" "" { target *-*-* } .-1 } */
+  /* { dg-note "'B::aaa' name already in use here" "" { target *-*-* } B_shadowing_aaa } */
+
+  // All branches of a multiple inheritance tree must be explored.
+  int bbb; /* { dg-warning "'Bi::bbb' might shadow 'D::bbb'" } */
+  /* { dg-note "'D::bbb' name already in use here" "" { target *-*-* } D_def_bbb } */
+  /* { dg-warning "'Bi::bbb' might shadow 'B::bbb'" "" { target *-*-* } .-2 } */
+  
+  // Must continue beyond the immediate parent, even in the case of multiple inheritance.
+  int ddd; /* { dg-warning "'Bi::ddd' might shadow 'D::ddd'" } */
+  /* { dg-note "'D::ddd' name already in use here" "" { target *-*-* } D_def_ddd } */
+};
+
+class BiD : public Bi {
+  /* If the base class does not cause a warning,
+  then look into each base classes of the parent's multiple inheritance */
+  int eee; /* { dg-warning "'BiD::eee' might shadow 'B::eee'" } */
+  /* { dg-note "'B::eee' name already in use here" "" { target *-*-* } B_def_eee } */
+  /* { dg-warning "'BiD::eee' might shadow 'E::eee'" "" { target *-*-* } .-2 } */
+  /* { dg-note "'E::eee' name already in use here" "" { target *-*-* } E_def_eee } */
+};
diff --git a/gcc/testsuite/g++.dg/pr12341-2.C b/gcc/testsuite/g++.dg/pr12341-2.C
new file mode 100644
index 00000000000..2836ff780e5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr12341-2.C
@@ -0,0 +1,34 @@
+// PR c++/12341 test anonymous bit field
+/* { dg-do compile } */
+/* { dg-additional-options "-Wshadow" } */
+
+class B {
+protected:
+  unsigned int x : 5;
+  unsigned int : 2;
+  unsigned int y : 1;
+
+  union {
+    float uuu;
+    double vvv;
+  };
+};
+
+// Check that anonymous bit fields do not cause spurious warnings
+class D : private B {
+protected:
+  unsigned int x : 4; /* { dg-warning "'D::x' might shadow 'B::x'" } */
+  unsigned int : 4; // If test for excess errors fails, it might be from here.
+  int uuu; /* { dg-warning "'D::uuu' might shadow 'B::<unnamed union>::uuu'" } */
+};
+
+class E : public D {
+public:
+  /* Do not warn if inheritance is not visible to the class,
+  as there is no risk even with polymorphism to mistake the fields. */
+  unsigned int y; /* { dg-bogus "'E::y' might shadow 'B::y'" } */
+  unsigned int vvv; /* { dg-bogus "'E::vvv' might shadow 'B::<unnamed union>::vvv'" } */
+
+  // Do warn even if the type differs. Should be trivial, still test for it.
+  double x; /* { dg-warning "'E::x' might shadow 'D::x'" } */
+};
-- 
2.34.1