[PATCH] c++: Extend -Wdangling-reference for std::minmax

2022-11-09 Thread Marek Polacek via Gcc-patches 
This patch extends -Wdangling-reference to also warn for

  auto v = std::minmax(1, 2);

which dangles because this overload of std::minmax returns
a std::pair where the two references are
bound to the temporaries created for the arguments of std::minmax.
This is a common footgun, also described at
 in Notes.

It works by extending do_warn_dangling_reference to also warn when the
function returns a std::pair.  std_pair_ref_ref_p
is a new helper to check that.

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

gcc/cp/ChangeLog:

* call.cc (std_pair_ref_ref_p): New.
(do_warn_dangling_reference): Also warn when the function returns
std::pair.  Recurse into TARGET_EXPR_INITIAL.
(maybe_warn_dangling_reference): Don't return early if we're
initializing a std_pair_ref_ref_p.

gcc/ChangeLog:

* doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst:
Extend the description of -Wdangling-reference.

gcc/testsuite/ChangeLog:

* g++.dg/warn/Wdangling-reference6.C: New test.
---
 gcc/cp/call.cc| 52 ---
 .../options-controlling-c++-dialect.rst   | 10 
 .../g++.dg/warn/Wdangling-reference6.C| 38 ++
 3 files changed, 94 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference6.C

diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index 492db9b59ad..bd3b64a7e26 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -13527,6 +13527,34 @@ initialize_reference (tree type, tree expr,
   return expr;
 }
 
+/* Return true if T is std::pair.  */
+
+static bool
+std_pair_ref_ref_p (tree t)
+{
+  /* First, check if we have std::pair.  */
+  if (!NON_UNION_CLASS_TYPE_P (t)
+  || !CLASSTYPE_TEMPLATE_INSTANTIATION (t))
+return false;
+  tree tdecl = TYPE_NAME (TYPE_MAIN_VARIANT (t));
+  if (!decl_in_std_namespace_p (tdecl))
+return false;
+  tree name = DECL_NAME (tdecl);
+  if (!name || !id_equal (name, "pair"))
+return false;
+
+  /* Now see if the template arguments are both const T&.  */
+  tree args = CLASSTYPE_TI_ARGS (t);
+  if (TREE_VEC_LENGTH (args) != 2)
+return false;
+  for (int i = 0; i < 2; i++)
+if (!TYPE_REF_OBJ_P (TREE_VEC_ELT (args, i))
+   || !CP_TYPE_CONST_P (TREE_TYPE (TREE_VEC_ELT (args, i
+  return false;
+
+  return true;
+}
+
 /* Helper for maybe_warn_dangling_reference to find a problematic CALL_EXPR
that initializes the LHS (and at least one of its arguments represents
a temporary, as outlined in maybe_warn_dangling_reference), or NULL_TREE
@@ -13556,11 +13584,6 @@ do_warn_dangling_reference (tree expr)
|| warning_suppressed_p (fndecl, OPT_Wdangling_reference)
|| !warning_enabled_at (DECL_SOURCE_LOCATION (fndecl),
OPT_Wdangling_reference)
-   /* If the function doesn't return a reference, don't warn.  This
-  can be e.g.
-const int& z = std::min({1, 2, 3, 4, 5, 6, 7});
-  which doesn't dangle: std::min here returns an int.  */
-   || !TYPE_REF_OBJ_P (TREE_TYPE (TREE_TYPE (fndecl)))
/* Don't emit a false positive for:
std::vector v = ...;
std::vector::const_iterator it = v.begin();
@@ -13573,6 +13596,20 @@ do_warn_dangling_reference (tree expr)
&& DECL_OVERLOADED_OPERATOR_IS (fndecl, INDIRECT_REF)))
  return NULL_TREE;
 
+   tree rettype = TREE_TYPE (TREE_TYPE (fndecl));
+   /* If the function doesn't return a reference, don't warn.  This
+  can be e.g.
+const int& z = std::min({1, 2, 3, 4, 5, 6, 7});
+  which doesn't dangle: std::min here returns an int.
+
+  If the function returns a std::pair, we
+  warn, to detect e.g.
+std::pair v = std::minmax(1, 2);
+  which also creates a dangling reference, because std::minmax
+  returns std::pair(b, a).  */
+   if (!(TYPE_REF_OBJ_P (rettype) || std_pair_ref_ref_p (rettype)))
+ return NULL_TREE;
+
/* Here we're looking to see if any of the arguments is a temporary
   initializing a reference parameter.  */
for (int i = 0; i < call_expr_nargs (expr); ++i)
@@ -13614,6 +13651,8 @@ do_warn_dangling_reference (tree expr)
   return do_warn_dangling_reference (TREE_OPERAND (expr, 2));
 case PAREN_EXPR:
   return do_warn_dangling_reference (TREE_OPERAND (expr, 0));
+case TARGET_EXPR:
+  return do_warn_dangling_reference (TARGET_EXPR_INITIAL (expr));
 default:
   return NULL_TREE;
 }
@@ -13640,7 +13679,8 @@ maybe_warn_dangling_reference (const_tree decl, tree 
init)
 {
   if (!warn_dangling_reference)
 return;
-  if (!TYPE_REF_P (TREE_TYPE (decl)))
+  if (!(TYPE_REF_OBJ_P (TREE_TYPE (decl))
+   || std_pair_ref_ref_p (TREE_TYPE (decl
 return;
   /* Don't

Re: [PATCH] c++: Extend -Wdangling-reference for std::minmax

2022-11-10 Thread Jason Merrill via Gcc-patches 

On 11/9/22 15:56, Marek Polacek wrote:

This patch extends -Wdangling-reference to also warn for

   auto v = std::minmax(1, 2);

which dangles because this overload of std::minmax returns
a std::pair where the two references are
bound to the temporaries created for the arguments of std::minmax.
This is a common footgun, also described at
 in Notes.

It works by extending do_warn_dangling_reference to also warn when the
function returns a std::pair.  std_pair_ref_ref_p
is a new helper to check that.

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

gcc/cp/ChangeLog:

* call.cc (std_pair_ref_ref_p): New.
(do_warn_dangling_reference): Also warn when the function returns
std::pair.  Recurse into TARGET_EXPR_INITIAL.
(maybe_warn_dangling_reference): Don't return early if we're
initializing a std_pair_ref_ref_p.

gcc/ChangeLog:

* doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst:
Extend the description of -Wdangling-reference.

gcc/testsuite/ChangeLog:

* g++.dg/warn/Wdangling-reference6.C: New test.
---
  gcc/cp/call.cc| 52 ---
  .../options-controlling-c++-dialect.rst   | 10 
  .../g++.dg/warn/Wdangling-reference6.C| 38 ++
  3 files changed, 94 insertions(+), 6 deletions(-)
  create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference6.C

diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index 492db9b59ad..bd3b64a7e26 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -13527,6 +13527,34 @@ initialize_reference (tree type, tree expr,
return expr;
  }
  
+/* Return true if T is std::pair.  */

+
+static bool
+std_pair_ref_ref_p (tree t)
+{
+  /* First, check if we have std::pair.  */
+  if (!NON_UNION_CLASS_TYPE_P (t)
+  || !CLASSTYPE_TEMPLATE_INSTANTIATION (t))
+return false;
+  tree tdecl = TYPE_NAME (TYPE_MAIN_VARIANT (t));
+  if (!decl_in_std_namespace_p (tdecl))
+return false;
+  tree name = DECL_NAME (tdecl);
+  if (!name || !id_equal (name, "pair"))
+return false;
+
+  /* Now see if the template arguments are both const T&.  */
+  tree args = CLASSTYPE_TI_ARGS (t);
+  if (TREE_VEC_LENGTH (args) != 2)
+return false;
+  for (int i = 0; i < 2; i++)
+if (!TYPE_REF_OBJ_P (TREE_VEC_ELT (args, i))
+   || !CP_TYPE_CONST_P (TREE_TYPE (TREE_VEC_ELT (args, i
+  return false;
+
+  return true;
+}
+
  /* Helper for maybe_warn_dangling_reference to find a problematic CALL_EXPR
 that initializes the LHS (and at least one of its arguments represents
 a temporary, as outlined in maybe_warn_dangling_reference), or NULL_TREE
@@ -13556,11 +13584,6 @@ do_warn_dangling_reference (tree expr)
|| warning_suppressed_p (fndecl, OPT_Wdangling_reference)
|| !warning_enabled_at (DECL_SOURCE_LOCATION (fndecl),
OPT_Wdangling_reference)
-   /* If the function doesn't return a reference, don't warn.  This
-  can be e.g.
-const int& z = std::min({1, 2, 3, 4, 5, 6, 7});
-  which doesn't dangle: std::min here returns an int.  */
-   || !TYPE_REF_OBJ_P (TREE_TYPE (TREE_TYPE (fndecl)))
/* Don't emit a false positive for:
std::vector v = ...;
std::vector::const_iterator it = v.begin();
@@ -13573,6 +13596,20 @@ do_warn_dangling_reference (tree expr)
&& DECL_OVERLOADED_OPERATOR_IS (fndecl, INDIRECT_REF)))
  return NULL_TREE;
  
+	tree rettype = TREE_TYPE (TREE_TYPE (fndecl));

+   /* If the function doesn't return a reference, don't warn.  This
+  can be e.g.
+const int& z = std::min({1, 2, 3, 4, 5, 6, 7});
+  which doesn't dangle: std::min here returns an int.
+
+  If the function returns a std::pair, we
+  warn, to detect e.g.
+std::pair v = std::minmax(1, 2);
+  which also creates a dangling reference, because std::minmax
+  returns std::pair(b, a).  */
+   if (!(TYPE_REF_OBJ_P (rettype) || std_pair_ref_ref_p (rettype)))


The patch is OK, but do you want to check reference to const for the 
single ref case as well, while you're changing this?



+ return NULL_TREE;
+
/* Here we're looking to see if any of the arguments is a temporary
   initializing a reference parameter.  */
for (int i = 0; i < call_expr_nargs (expr); ++i)
@@ -13614,6 +13651,8 @@ do_warn_dangling_reference (tree expr)
return do_warn_dangling_reference (TREE_OPERAND (expr, 2));
  case PAREN_EXPR:
return do_warn_dangling_reference (TREE_OPERAND (expr, 0));
+case TARGET_EXPR:
+  return do_warn_dangling_reference (TARGET_EXPR_INITIAL (expr));
  default:
return NULL_TREE;
  }
@@ -13640,7 +13679,8 @@ maybe_warn_dangling_reference (const_tree decl, tree 
init)
  {
if (!warn_dang

Re: [PATCH] c++: Extend -Wdangling-reference for std::minmax

2022-11-10 Thread Marek Polacek via Gcc-patches 
On Thu, Nov 10, 2022 at 08:07:25AM -1000, Jason Merrill wrote:
> On 11/9/22 15:56, Marek Polacek wrote:
> > This patch extends -Wdangling-reference to also warn for
> > 
> >auto v = std::minmax(1, 2);
> > 
> > which dangles because this overload of std::minmax returns
> > a std::pair where the two references are
> > bound to the temporaries created for the arguments of std::minmax.
> > This is a common footgun, also described at
> >  in Notes.
> > 
> > It works by extending do_warn_dangling_reference to also warn when the
> > function returns a std::pair.  std_pair_ref_ref_p
> > is a new helper to check that.
> > 
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > 
> > gcc/cp/ChangeLog:
> > 
> > * call.cc (std_pair_ref_ref_p): New.
> > (do_warn_dangling_reference): Also warn when the function returns
> > std::pair.  Recurse into TARGET_EXPR_INITIAL.
> > (maybe_warn_dangling_reference): Don't return early if we're
> > initializing a std_pair_ref_ref_p.
> > 
> > gcc/ChangeLog:
> > 
> > * doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst:
> > Extend the description of -Wdangling-reference.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > * g++.dg/warn/Wdangling-reference6.C: New test.
> > ---
> >   gcc/cp/call.cc| 52 ---
> >   .../options-controlling-c++-dialect.rst   | 10 
> >   .../g++.dg/warn/Wdangling-reference6.C| 38 ++
> >   3 files changed, 94 insertions(+), 6 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference6.C
> > 
> > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> > index 492db9b59ad..bd3b64a7e26 100644
> > --- a/gcc/cp/call.cc
> > +++ b/gcc/cp/call.cc
> > @@ -13527,6 +13527,34 @@ initialize_reference (tree type, tree expr,
> > return expr;
> >   }
> > +/* Return true if T is std::pair.  */
> > +
> > +static bool
> > +std_pair_ref_ref_p (tree t)
> > +{
> > +  /* First, check if we have std::pair.  */
> > +  if (!NON_UNION_CLASS_TYPE_P (t)
> > +  || !CLASSTYPE_TEMPLATE_INSTANTIATION (t))
> > +return false;
> > +  tree tdecl = TYPE_NAME (TYPE_MAIN_VARIANT (t));
> > +  if (!decl_in_std_namespace_p (tdecl))
> > +return false;
> > +  tree name = DECL_NAME (tdecl);
> > +  if (!name || !id_equal (name, "pair"))
> > +return false;
> > +
> > +  /* Now see if the template arguments are both const T&.  */
> > +  tree args = CLASSTYPE_TI_ARGS (t);
> > +  if (TREE_VEC_LENGTH (args) != 2)
> > +return false;
> > +  for (int i = 0; i < 2; i++)
> > +if (!TYPE_REF_OBJ_P (TREE_VEC_ELT (args, i))
> > +   || !CP_TYPE_CONST_P (TREE_TYPE (TREE_VEC_ELT (args, i
> > +  return false;
> > +
> > +  return true;
> > +}
> > +
> >   /* Helper for maybe_warn_dangling_reference to find a problematic 
> > CALL_EXPR
> >  that initializes the LHS (and at least one of its arguments represents
> >  a temporary, as outlined in maybe_warn_dangling_reference), or 
> > NULL_TREE
> > @@ -13556,11 +13584,6 @@ do_warn_dangling_reference (tree expr)
> > || warning_suppressed_p (fndecl, OPT_Wdangling_reference)
> > || !warning_enabled_at (DECL_SOURCE_LOCATION (fndecl),
> > OPT_Wdangling_reference)
> > -   /* If the function doesn't return a reference, don't warn.  This
> > -  can be e.g.
> > -const int& z = std::min({1, 2, 3, 4, 5, 6, 7});
> > -  which doesn't dangle: std::min here returns an int.  */
> > -   || !TYPE_REF_OBJ_P (TREE_TYPE (TREE_TYPE (fndecl)))
> > /* Don't emit a false positive for:
> > std::vector v = ...;
> > std::vector::const_iterator it = v.begin();
> > @@ -13573,6 +13596,20 @@ do_warn_dangling_reference (tree expr)
> > && DECL_OVERLOADED_OPERATOR_IS (fndecl, INDIRECT_REF)))
> >   return NULL_TREE;
> > +   tree rettype = TREE_TYPE (TREE_TYPE (fndecl));
> > +   /* If the function doesn't return a reference, don't warn.  This
> > +  can be e.g.
> > +const int& z = std::min({1, 2, 3, 4, 5, 6, 7});
> > +  which doesn't dangle: std::min here returns an int.
> > +
> > +  If the function returns a std::pair, we
> > +  warn, to detect e.g.
> > +std::pair v = std::minmax(1, 2);
> > +  which also creates a dangling reference, because std::minmax
> > +  returns std::pair(b, a).  */
> > +   if (!(TYPE_REF_OBJ_P (rettype) || std_pair_ref_ref_p (rettype)))
> 
> The patch is OK, but do you want to check reference to const for the single
> ref case as well, while you're changing this?

Thanks.  Yes, I plan to do that soon, but I didn't want to do it in
a single patch, because I want a dedicated test for 'int&' v. 'const int&'
and that felt like a follow-up patch.
 
> > + return NULL_TREE;
> > +
> > /* Here we're looking to see if any of the arguments is a temporary
> >initializing a reference