Re: [PATCH] c++: Additional warning for name-hiding [PR12341]
On 9/4/23 13:18, priour...@gmail.com wrote: From: benjamin priour 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 --- 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 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
[PATCH] c++: Additional warning for name-hiding [PR12341]
From: benjamin priour 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 --- 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 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 @@
[PATCH] c++: Additional warning for name-hiding [PR12341]
Hi everyone, My first patch, and I don't have write access yet. This patch add a new warning under -Wshadow, to warn when a class field hides another inherited. At the moment, I'm looking for a similarly named field independently of its visibility (whether it is public, protected or private within the base class). However, if the inheritance itself is not visible from the current class, then I dismiss the warning (e.g. Grandparent class is inherited privately by Parent, then Child won't collide). Note that ChangeLogs were generated without the script `git gcc-mklog`. Bootstrapped and regtested on x86_64-pc-linux-gnu. Diff is with 20230413-trunk, I checked in with today 20230416 trunk though. I tried to follow the contribute page down to the letter, still if I've missed anything, please tell me how I could improve the submission. Great thanks, Benjamin. c++: Additional warning for name-hiding [PR12341] Add a new warning for name-hiding. When a class's field is named similarly to one inherited, a warning should be issued. 2023-04-13 Benjamin Priour vutlk...@gcc.gnu.org gcc/cp/ChangeLog: * search.cc (lookup_member): New optional parameter to preempt too deep inheritance tree processing. (lookup_field): Likewise. (dfs_walk_all): Likewise. * cp-tree.h: Complete the above declarations. * class.cc (warn_name_hiding): New function. (finish_struct_1): Call warn_name_hiding if -Wshadow. gcc/testsuite/ChangeLog: * g++.dg/warn/pr12341-1.C: New file. * g++.dg/warn/pr12341-2.C: New file. --- diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc index 68b62086340..1e3efc028a6 100644 --- a/gcc/cp/class.cc +++ b/gcc/cp/class.cc @@ -3080,6 +3080,80 @@ 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 a 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 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 +7728,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 622752ae4e6..e56e85d93cc 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7519,11 +7519,11 @@ 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