diff mbox series

c++: further -Wdangling-reference refinement [PR107532]

Message ID 20230317202908.42800-1-polacek@redhat.com
State New
Headers show
Series c++: further -Wdangling-reference refinement [PR107532] | expand

Commit Message

Marek Polacek March 17, 2023, 8:29 p.m. UTC
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));

Since reference_like_class_p already checks for std::pair<const T&, const T&>
maybe it could also check for std::tuple<T&, ...>.  I don't know if we
want to make that change in GCC 13, or move -Wdangling-reference to
-Wextra for GCC 13 and perhaps move it back to -Wall in GCC 14.

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

	PR c++/107532

gcc/cp/ChangeLog:

	* call.cc (reference_like_class_p): Don't look for a constructor.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Wdangling-reference11.C: New test.
---
 gcc/cp/call.cc                                | 35 +++++++------------
 .../g++.dg/warn/Wdangling-reference11.C       | 23 ++++++++++++
 2 files changed, 35 insertions(+), 23 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference11.C


base-commit: ae7190e345a8d80310835cb83b3b41ef2aeb0d37

Comments

Jason Merrill March 18, 2023, 12:35 p.m. UTC | #1
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?

> Since reference_like_class_p already checks for std::pair<const T&, const T&>
> maybe it could also check for std::tuple<T&, ...>.  I don't know if we
> want to make that change in GCC 13, or move -Wdangling-reference to
> -Wextra for GCC 13 and perhaps move it back to -Wall in GCC 14.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> 	PR c++/107532
> 
> gcc/cp/ChangeLog:
> 
> 	* call.cc (reference_like_class_p): Don't look for a constructor.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/warn/Wdangling-reference11.C: New test.
> ---
>   gcc/cp/call.cc                                | 35 +++++++------------
>   .../g++.dg/warn/Wdangling-reference11.C       | 23 ++++++++++++
>   2 files changed, 35 insertions(+), 23 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference11.C
> 
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index c01e7b82457..00d56a157b6 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -13781,8 +13781,9 @@ 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
>   reference_like_class_p (tree ctype)
> @@ -13798,31 +13799,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")));
> +      if (name
> +	  && (id_equal (name, "reference_wrapper")
> +	      || id_equal (name, "span")
> +	      || id_equal (name, "ref_view")))
> +	return true;
>       }
>     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 (TREE_CODE (fields) == FIELD_DECL
> +	&& !DECL_ARTIFICIAL (fields)
> +	&& TYPE_REF_P (TREE_TYPE (fields)))
> +      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;
> +}
> 
> base-commit: ae7190e345a8d80310835cb83b3b41ef2aeb0d37
diff mbox series

Patch

diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index c01e7b82457..00d56a157b6 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -13781,8 +13781,9 @@  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
 reference_like_class_p (tree ctype)
@@ -13798,31 +13799,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")));
+      if (name
+	  && (id_equal (name, "reference_wrapper")
+	      || id_equal (name, "span")
+	      || id_equal (name, "ref_view")))
+	return true;
     }
   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 (TREE_CODE (fields) == FIELD_DECL
+	&& !DECL_ARTIFICIAL (fields)
+	&& TYPE_REF_P (TREE_TYPE (fields)))
+      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;
+}