[v2] c++: further -Wdangling-reference refinement [PR107532]
Commit Message
On Sat, Mar 18, 2023 at 08:35:36AM -0400, Jason Merrill wrote:
> On 3/17/23 16:29, Marek Polacek wrote:
> > Based on <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532#c24>,
> > it seems like we should treat *any* class with a reference member
> > as a reference wrapper. This simplifies the code so I'm happy to
> > make that change.
> >
> > The patch, however, does not suppress the warning in
> >
> > int i = 42;
> > auto const& v = std::get<0>(std::tuple<int&>(i));
>
> Why not? tuple<int&> has an int& member, doesn't it? Do we need to look
> into bases as well?
Indeed. I don't know why I didn't do it right away; it's really not that
complicated:
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
-- >8 --
Based on <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532#c24>,
it seems like we should treat *any* class with a reference member
as a reference wrapper. To suppress the warning in
int i = 42;
auto const& v = std::get<0>(std::tuple<int&>(i));
we have to look into base classes as well. For std::tuple, this means
that we have to check the _Head_base subobject, which is a non-direct
base class of std::tuple. So I've employed a DFS walk.
PR c++/107532
gcc/cp/ChangeLog:
* call.cc (class_has_reference_member_p): New.
(class_has_reference_member_p_r): New.
(reference_like_class_p): Don't look for a specific constructor.
Use a DFS walk with class_has_reference_member_p_r.
gcc/testsuite/ChangeLog:
* g++.dg/warn/Wdangling-reference11.C: New test.
* g++.dg/warn/Wdangling-reference12.C: New test.
---
gcc/cp/call.cc | 63 +++++++++++--------
.../g++.dg/warn/Wdangling-reference11.C | 23 +++++++
.../g++.dg/warn/Wdangling-reference12.C | 12 ++++
3 files changed, 72 insertions(+), 26 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference11.C
create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference12.C
base-commit: a226590fefb35ed66adf73d85cefe49048a78ab8
Comments
On 3/20/23 18:06, Marek Polacek wrote:
> On Sat, Mar 18, 2023 at 08:35:36AM -0400, Jason Merrill wrote:
>> On 3/17/23 16:29, Marek Polacek wrote:
>>> Based on <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532#c24>,
>>> it seems like we should treat *any* class with a reference member
>>> as a reference wrapper. This simplifies the code so I'm happy to
>>> make that change.
>>>
>>> The patch, however, does not suppress the warning in
>>>
>>> int i = 42;
>>> auto const& v = std::get<0>(std::tuple<int&>(i));
>>
>> Why not? tuple<int&> has an int& member, doesn't it? Do we need to look
>> into bases as well?
>
> Indeed. I don't know why I didn't do it right away; it's really not that
> complicated:
>
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
OK.
> -- >8 --
> Based on <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532#c24>,
> it seems like we should treat *any* class with a reference member
> as a reference wrapper. To suppress the warning in
>
> int i = 42;
> auto const& v = std::get<0>(std::tuple<int&>(i));
>
> we have to look into base classes as well. For std::tuple, this means
> that we have to check the _Head_base subobject, which is a non-direct
> base class of std::tuple. So I've employed a DFS walk.
>
> PR c++/107532
>
> gcc/cp/ChangeLog:
>
> * call.cc (class_has_reference_member_p): New.
> (class_has_reference_member_p_r): New.
> (reference_like_class_p): Don't look for a specific constructor.
> Use a DFS walk with class_has_reference_member_p_r.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/warn/Wdangling-reference11.C: New test.
> * g++.dg/warn/Wdangling-reference12.C: New test.
> ---
> gcc/cp/call.cc | 63 +++++++++++--------
> .../g++.dg/warn/Wdangling-reference11.C | 23 +++++++
> .../g++.dg/warn/Wdangling-reference12.C | 12 ++++
> 3 files changed, 72 insertions(+), 26 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference11.C
> create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference12.C
>
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index c52a09b9be2..429170e43ea 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -13783,8 +13783,31 @@ std_pair_ref_ref_p (tree t)
>
> /* Return true if a class CTYPE is either std::reference_wrapper or
> std::ref_view, or a reference wrapper class. We consider a class
> - a reference wrapper class if it has a reference member and a
> - constructor taking the same reference type. */
> + a reference wrapper class if it has a reference member. We no
> + longer check that it has a constructor taking the same reference type
> + since that approach still generated too many false positives. */
> +
> +static bool
> +class_has_reference_member_p (tree t)
> +{
> + for (tree fields = TYPE_FIELDS (t);
> + fields;
> + fields = DECL_CHAIN (fields))
> + if (TREE_CODE (fields) == FIELD_DECL
> + && !DECL_ARTIFICIAL (fields)
> + && TYPE_REF_P (TREE_TYPE (fields)))
> + return true;
> + return false;
> +}
> +
> +/* A wrapper for the above suitable as a callback for dfs_walk_once. */
> +
> +static tree
> +class_has_reference_member_p_r (tree binfo, void *)
> +{
> + return (class_has_reference_member_p (BINFO_TYPE (binfo))
> + ? integer_one_node : NULL_TREE);
> +}
>
> static bool
> reference_like_class_p (tree ctype)
> @@ -13800,31 +13823,19 @@ reference_like_class_p (tree ctype)
> if (decl_in_std_namespace_p (tdecl))
> {
> tree name = DECL_NAME (tdecl);
> - return (name
> - && (id_equal (name, "reference_wrapper")
> - || id_equal (name, "span")
> - || id_equal (name, "ref_view")));
> - }
> - for (tree fields = TYPE_FIELDS (ctype);
> - fields;
> - fields = DECL_CHAIN (fields))
> - {
> - if (TREE_CODE (fields) != FIELD_DECL || DECL_ARTIFICIAL (fields))
> - continue;
> - tree type = TREE_TYPE (fields);
> - if (!TYPE_REF_P (type))
> - continue;
> - /* OK, the field is a reference member. Do we have a constructor
> - taking its type? */
> - for (tree fn : ovl_range (CLASSTYPE_CONSTRUCTORS (ctype)))
> - {
> - tree args = FUNCTION_FIRST_USER_PARMTYPE (fn);
> - if (args
> - && same_type_p (TREE_VALUE (args), type)
> - && TREE_CHAIN (args) == void_list_node)
> - return true;
> - }
> + if (name
> + && (id_equal (name, "reference_wrapper")
> + || id_equal (name, "span")
> + || id_equal (name, "ref_view")))
> + return true;
> }
> +
> + /* Some classes, such as std::tuple, have the reference member in its
> + (non-direct) base class. */
> + if (dfs_walk_once (TYPE_BINFO (ctype), class_has_reference_member_p_r,
> + nullptr, nullptr))
> + return true;
> +
> return false;
> }
>
> diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-reference11.C b/gcc/testsuite/g++.dg/warn/Wdangling-reference11.C
> new file mode 100644
> index 00000000000..667618e7196
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wdangling-reference11.C
> @@ -0,0 +1,23 @@
> +// PR c++/107532
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wdangling-reference" }
> +
> +struct R
> +{
> + int& r;
> + int& get() { return r; }
> + int&& rget() { return static_cast<int&&>(r); }
> +};
> +
> +int main()
> +{
> + int i = 42;
> + int& l = R{i}.get(); // { dg-bogus "dangling reference" }
> + int const& cl = R{i}.get(); // { dg-bogus "dangling reference" }
> + int&& r = R{i}.rget(); // { dg-bogus "dangling reference" }
> + int const&& cr = R{i}.rget(); // { dg-bogus "dangling reference" }
> + (void) l;
> + (void) r;
> + (void) cr;
> + (void) cl;
> +}
> diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-reference12.C b/gcc/testsuite/g++.dg/warn/Wdangling-reference12.C
> new file mode 100644
> index 00000000000..85e01f01a50
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wdangling-reference12.C
> @@ -0,0 +1,12 @@
> +// PR c++/107532
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wdangling-reference" }
> +
> +#include <tuple>
> +
> +int main()
> +{
> + int i = 42;
> + auto const& v = std::get<0>(std::tuple<int&>(i)); // { dg-bogus "dangling reference" }
> + (void) v;
> +}
>
> base-commit: a226590fefb35ed66adf73d85cefe49048a78ab8
@@ -13783,8 +13783,31 @@ std_pair_ref_ref_p (tree t)
/* Return true if a class CTYPE is either std::reference_wrapper or
std::ref_view, or a reference wrapper class. We consider a class
- a reference wrapper class if it has a reference member and a
- constructor taking the same reference type. */
+ a reference wrapper class if it has a reference member. We no
+ longer check that it has a constructor taking the same reference type
+ since that approach still generated too many false positives. */
+
+static bool
+class_has_reference_member_p (tree t)
+{
+ for (tree fields = TYPE_FIELDS (t);
+ fields;
+ fields = DECL_CHAIN (fields))
+ if (TREE_CODE (fields) == FIELD_DECL
+ && !DECL_ARTIFICIAL (fields)
+ && TYPE_REF_P (TREE_TYPE (fields)))
+ return true;
+ return false;
+}
+
+/* A wrapper for the above suitable as a callback for dfs_walk_once. */
+
+static tree
+class_has_reference_member_p_r (tree binfo, void *)
+{
+ return (class_has_reference_member_p (BINFO_TYPE (binfo))
+ ? integer_one_node : NULL_TREE);
+}
static bool
reference_like_class_p (tree ctype)
@@ -13800,31 +13823,19 @@ reference_like_class_p (tree ctype)
if (decl_in_std_namespace_p (tdecl))
{
tree name = DECL_NAME (tdecl);
- return (name
- && (id_equal (name, "reference_wrapper")
- || id_equal (name, "span")
- || id_equal (name, "ref_view")));
- }
- for (tree fields = TYPE_FIELDS (ctype);
- fields;
- fields = DECL_CHAIN (fields))
- {
- if (TREE_CODE (fields) != FIELD_DECL || DECL_ARTIFICIAL (fields))
- continue;
- tree type = TREE_TYPE (fields);
- if (!TYPE_REF_P (type))
- continue;
- /* OK, the field is a reference member. Do we have a constructor
- taking its type? */
- for (tree fn : ovl_range (CLASSTYPE_CONSTRUCTORS (ctype)))
- {
- tree args = FUNCTION_FIRST_USER_PARMTYPE (fn);
- if (args
- && same_type_p (TREE_VALUE (args), type)
- && TREE_CHAIN (args) == void_list_node)
- return true;
- }
+ if (name
+ && (id_equal (name, "reference_wrapper")
+ || id_equal (name, "span")
+ || id_equal (name, "ref_view")))
+ return true;
}
+
+ /* Some classes, such as std::tuple, have the reference member in its
+ (non-direct) base class. */
+ if (dfs_walk_once (TYPE_BINFO (ctype), class_has_reference_member_p_r,
+ nullptr, nullptr))
+ return true;
+
return false;
}
new file mode 100644
@@ -0,0 +1,23 @@
+// PR c++/107532
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wdangling-reference" }
+
+struct R
+{
+ int& r;
+ int& get() { return r; }
+ int&& rget() { return static_cast<int&&>(r); }
+};
+
+int main()
+{
+ int i = 42;
+ int& l = R{i}.get(); // { dg-bogus "dangling reference" }
+ int const& cl = R{i}.get(); // { dg-bogus "dangling reference" }
+ int&& r = R{i}.rget(); // { dg-bogus "dangling reference" }
+ int const&& cr = R{i}.rget(); // { dg-bogus "dangling reference" }
+ (void) l;
+ (void) r;
+ (void) cr;
+ (void) cl;
+}
new file mode 100644
@@ -0,0 +1,12 @@
+// PR c++/107532
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wdangling-reference" }
+
+#include <tuple>
+
+int main()
+{
+ int i = 42;
+ auto const& v = std::get<0>(std::tuple<int&>(i)); // { dg-bogus "dangling reference" }
+ (void) v;
+}