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

[Jason Merrill <jason@redhat.com>]

On 9/4/23 13:18, priour.be@gmail.com wrote:
> 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++)

Rather than iterate through the bases here, maybe add a mode to 
lookup_member/lookup_field_r that skips the most derived type, e.g. by 
adding that as a flag in lookup_field_info?

Probably instead of the once_suffices stuff?

> +	  if (base_vardecl)
> +	    {
> +	      auto_diagnostic_group d;
> +	      if (warning_at (location_of (field), OPT_Wshadow,
> +			      "%qD might shadow %qD", field, base_vardecl))

Why "might"?  We can give a correct answer, we shouldn't settle for an 
approximation.

Jason