On 3/29/23 07:36, Benjamin Priour wrote:
Hi, below is my first patch ever. I ran the testsuites against trunk
20230322, everything seems OK to me, but as it is my first submission
I'd like to be sure of it.
Thanks a lot for the review !
Thanks!
Please see https://gcc.gnu.org/contribute.html#patches for more info on
submitting patches. Specifically, the subject line needs a component
tag, and the body of the message needs more description of the change.
Instead of writing your own lookup_basevardecls you might want to adjust
lookup_member/lookup_field_r to have a mode that skips the most derived
type (lfi->type).
Jason
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index 68b62086340..147a7458488 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -3080,6 +3080,109 @@ warn_hidden (tree t)
}
}
+
+/* Lookup the inheritance hierarchy of T for any non-static field that have
+ the indicated NAME. */
+static void
+lookup_basevardecls (tree name, tree t, vec<tree> *base_vardecls)
+{
+ /* Find non-static fields in T with the indicated NAME. */
+ for (tree field = TYPE_FIELDS(t); field; field = DECL_CHAIN(field))
In GCC we put a space before (
+ {
+ /* Going with the same policy as warn_hidden with base class's
members
+ non visible from child, i.e. do not distinguish.
+ A justification might be to not omit warning in the following case:
+
+ class Ancestor {
+ friend void foo_accessor(Ancestor *);
+
+ private:
+ int x;
+ };
+
+ class Descendant : public Ancestor {
+ public:
+ int x;
+ }
+
+ void foo_accessor(Ancestor *anc) {
+ ...
+ anc->x;
+ ...
+ }
+
+ foo_accessor(new Descendant());
+ */
+ if (TREE_CODE (field) == FIELD_DECL
+ && !DECL_ARTIFICIAL(field)
+ && DECL_NAME(field) == name)
+ {
+ base_vardecls->safe_push (field);
+ /* Return upon first match, as there cannot be two data members
+ named equally in the same RECORD_TYPE.
+ Moreover, avoid redundant warnings by not diving deeper into
+ T inheritance hierarchy. */
+ return;
+ }
+ }
+
+ int n_baseclasses = BINFO_N_BASE_BINFOS (TYPE_BINFO (t));
+ /* Go one step up the inheritance hierarchy. */
+ for (int i = 0; i < n_baseclasses; i++)
+ {
+ tree basetype = BINFO_TYPE (BINFO_BASE_BINFO (TYPE_BINFO (t), i));
+ lookup_basevardecls (name, basetype, base_vardecls);
+ }
+}
+
+
+/* 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 a user-defined non-static data member. */
+ if (TREE_CODE(field) != FIELD_DECL || DECL_ARTIFICIAL(field))
+ continue;
+
+ unsigned j;
+ tree name = DECL_NAME(field);
I think you also need to skip unnamed bit-fields.
+ /* Not sure about the size parameter of auto_vec */
+ 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);
+ lookup_basevardecls(name, basetype, &base_vardecls);
+ }
+
+ /* 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 +7757,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/testsuite/g++.dg/warn/pr12341-1.C
b/gcc/testsuite/g++.dg/warn/pr12341-1.C
new file mode 100644
index 00000000000..2c8f63d3b4f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/pr12341-1.C
@@ -0,0 +1,51 @@
+// PR c++/12341
+/* { dg-do compile } */
+/* { dg-options -Wshadow } */
+
+class A {
+protected:
+ int aaa; /* { dg-line A_def_aaa } */
+};
+
+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 } */
+};
+
+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;
+};
+
+// all first-level base classes must be considered.
+class Bi : protected B, private E {
+protected:
+ // If a match was already found, don't go further up the class hierarchy
+ 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 } */
+
+ // Every base classes 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 } */
+};
\ No newline at end of file
