From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x32a.google.com (mail-wm1-x32a.google.com [IPv6:2a00:1450:4864:20::32a]) by sourceware.org (Postfix) with ESMTPS id 9433A3858C30; Mon, 4 Sep 2023 17:23:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9433A3858C30 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-x32a.google.com with SMTP id 5b1f17b1804b1-401d10e3e54so16999735e9.2; Mon, 04 Sep 2023 10:23:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1693848238; x=1694453038; darn=gcc.gnu.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=Rkwb2jB+AX5+e/a1y5bm6sKX/tdszMA5UF4WHgLSX6A=; b=n18T3KZ0XKSfGroG2IJHkD3YeJTdoA/6Z/QL8EXfmUY5c3zHs+LyaYKylTQagRExLV xpgVZYQDYHlmy5R4jNFG3gefss5+cR41mxIaFUu9CdQlGi2nZa3a4pcTuhAjiyigAsN9 oT58oOx6rGjBukr8hNb2EOBGvQHIehTvvEomeholA5fsLrUTGmg6/MpzBkRCwhcrT1ex fy+tAsqPyOKwPy0YmGGg30z90TTePIFhd5MSLXisbTtYbbFdIo7TNkiL+DuN4JjOj9R3 UCiBYcVSud/0RuBRx6E+T56oOMS7K8ST9cVrWmR+OHYBMEfa936OYZ9KqCTG69Z2lzbK Ah6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693848238; x=1694453038; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Rkwb2jB+AX5+e/a1y5bm6sKX/tdszMA5UF4WHgLSX6A=; b=f1hHVouwMxQrUIxjD0uEi5p026sBfG1PDi/Cf/4Z/bFxQdt9vW8xHrGxDzZIcQTude iBvxBBaYy58n9v454v3ZKb45InRaGjbqsR525veLVW5zjgLDv5IROY2lJqJgj+b4AR14 XaOh01UlnnsYbYt6pf218tNBXw3fNwnlbHrjaAyb2Wixmp1YAPWuSTSNnlryahka1XA4 M8h9m22uMAZzDcYwDH0ReILynlOfbEkJhLQMJcnFFw8KoAKztUYonXnrVvYI24voptm6 A2XigxcLwLQp/g8J3EvgniWn9wyLQm2oBGkrTxMtrzawcV+1co+dSFmSsAnzJSG1ae16 Jn9A== X-Gm-Message-State: AOJu0Ywzr7Cja2mmcgA3zhqqmOI0IKRVgbyHD+OdhSJRnXOye7f/UG8N xdj0w3kchxD/aGt5sQCYFLQgSYoKM0Jd X-Google-Smtp-Source: AGHT+IHJeA5wIfF2ScGa5+aEXB4L51lLmac3xJGNWoerlIHeRYLHVicZ8KyRJrtU70bSC5uoH4ZO5g== X-Received: by 2002:a5d:67c7:0:b0:31a:e5e8:1d8e with SMTP id n7-20020a5d67c7000000b0031ae5e81d8emr7902728wrw.5.1693848237983; Mon, 04 Sep 2023 10:23:57 -0700 (PDT) Received: from localhost ([2a01:e0a:2ec:f0d0:e30e:67bf:23ff:df20]) by smtp.gmail.com with UTF8SMTPSA id o4-20020a5d6844000000b003197b85bad2sm15125010wrw.79.2023.09.04.10.23.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 04 Sep 2023 10:23:57 -0700 (PDT) From: priour.be@gmail.com X-Google-Original-From: vultkayn@gcc.gnu.org To: gcc-patches@gcc.gnu.org Cc: jason@redhat.com, benjamin priour Subject: [PATCH] c++: Additional warning for name-hiding [PR12341] Date: Mon, 4 Sep 2023 19:18:59 +0200 Message-Id: <20230904171858.2660517-1-vultkayn@gcc.gnu.org> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 @@ 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::::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::::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