[RFC] Fix for c++/PR12341

  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 !

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
  

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
>