Re: [PATCH v2] c++: -Wdangling-reference with reference wrapper [PR107532]
On 1/18/23 20:13, Marek Polacek wrote:
On Wed, Jan 18, 2023 at 04:07:59PM -0500, Jason Merrill wrote:
On 1/18/23 12:52, Marek Polacek wrote:
Here, -Wdangling-reference triggers where it probably shouldn't, causing
some grief. The code in question uses a reference wrapper with a member
function returning a reference to a subobject of a non-temporary object:
const Plane & meta = fm.planes().inner();
I've tried a few approaches, e.g., checking that the member function's
return type is the same as the type of the enclosing class (which is
the case for member functions returning *this), but that then breaks
Wdangling-reference4.C with std::optional.
So I figured that perhaps we want to look at the object we're invoking
the member function(s) on and see if that is a temporary, as in, don't
warn about
const Plane & meta = fm.planes().inner();
but do warn about
const Plane & meta = FrameMetadata().planes().inner();
It's ugly, but better than asking users to add #pragmas into their code.
Hmm, that doesn't seem right; the former is only OK because Ref is in fact a
reference-like type. If planes() returned a class that held data, we would
want to warn.
Sure, it's always some kind of tradeoff with warnings :/.
In this case, we might recognize the reference-like class because it has a
reference member and a constructor taking the same reference type.
That occurred to me too, but then I found out that std::reference_wrapper
actually uses T*, not T&, as you say. But here's a patch to do that
(I hope).
That wouldn't help with std::reference_wrapper or std::ref_view because they
have pointer members instead of references, but perhaps loosening the check
to include that case would make sense?
Sorry, I don't understand what you mean by loosening the check. I could
hardcode std::reference_wrapper and std::ref_view but I don't think that's
what you meant.
Indeed that's not what I meant, but as I was saying in our meeting I
think it's worth doing; the compiler has various tweaks to handle
specific standard-library classes better.
Surely I cannot _not_ warn for any class that contains a T*.
I was thinking if a constructor takes a T& and the class has a T* that
would be close enough, though this also wouldn't handle the standard
library classes so the benefit is questionable.
Here's the patch so that we have some actual code to discuss... Thanks.
-- >8 --
Here, -Wdangling-reference triggers where it probably shouldn't, causing
some grief. The code in question uses a reference wrapper with a member
function returning a reference to a subobject of a non-temporary object:
const Plane & meta = fm.planes().inner();
I've tried a few approaches, e.g., checking that the member function's
return type is the same as the type of the enclosing class (which is
the case for member functions returning *this), but that then breaks
Wdangling-reference4.C with std::optional.
Perhaps we want to look at the member function's enclosing class
to see if it's a reference wrapper class (meaning, has a reference
member and a constructor taking the same reference type) and don't
warn if so, supposing that the member function returns a reference
to a non-temporary object.
It's ugly, but better than asking users to add #pragmas into their code.
PR c++/107532
gcc/cp/ChangeLog:
* call.cc (do_warn_dangling_reference): Don't warn when the
member function comes from a reference wrapper class.
Let's factor the new code out into e.g. reference_like_class_p
gcc/testsuite/ChangeLog:
* g++.dg/warn/Wdangling-reference8.C: New test.
---
gcc/cp/call.cc| 32
.../g++.dg/warn/Wdangling-reference8.C| 77 +++
2 files changed, 109 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference8.C
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index 0780b5840a3..b0670a21240 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -13832,6 +13832,38 @@ do_warn_dangling_reference (tree expr)
if (!(TYPE_REF_OBJ_P (rettype) || std_pair_ref_ref_p (rettype)))
return NULL_TREE;
+ /* An attempt to reduce the number of -Wdangling-reference
+ false positives concerning reference wrappers (c++/107532).
+ If the enclosing class is a reference-like class, that is, has
+ a reference member and a constructor taking the same reference type,
+ we suppose that the member function is returning a reference
+ to a non-temporary object. */
+ if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fndecl)
+ && !DECL_OVERLOADED_OPERATOR_P (fndecl))
+ {
+ tree ctx = CP_DECL_CONTEXT (fndecl);
+ for (tree fields = TYPE_FIELDS (ctx);
+fields;
+fields = DECL_CHAIN (fields))
+ {
+ if (TREE_CODE (fields) != FIELD_DECL || DECL_ARTIFICIAL
(fields))
+ continue;
[PATCH v2] c++: -Wdangling-reference with reference wrapper [PR107532]
On Wed, Jan 18, 2023 at 04:07:59PM -0500, Jason Merrill wrote:
> On 1/18/23 12:52, Marek Polacek wrote:
> > Here, -Wdangling-reference triggers where it probably shouldn't, causing
> > some grief. The code in question uses a reference wrapper with a member
> > function returning a reference to a subobject of a non-temporary object:
> >
> >const Plane & meta = fm.planes().inner();
> >
> > I've tried a few approaches, e.g., checking that the member function's
> > return type is the same as the type of the enclosing class (which is
> > the case for member functions returning *this), but that then breaks
> > Wdangling-reference4.C with std::optional.
> >
> > So I figured that perhaps we want to look at the object we're invoking
> > the member function(s) on and see if that is a temporary, as in, don't
> > warn about
> >
> >const Plane & meta = fm.planes().inner();
> >
> > but do warn about
> >
> >const Plane & meta = FrameMetadata().planes().inner();
> >
> > It's ugly, but better than asking users to add #pragmas into their code.
>
> Hmm, that doesn't seem right; the former is only OK because Ref is in fact a
> reference-like type. If planes() returned a class that held data, we would
> want to warn.
Sure, it's always some kind of tradeoff with warnings :/.
> In this case, we might recognize the reference-like class because it has a
> reference member and a constructor taking the same reference type.
That occurred to me too, but then I found out that std::reference_wrapper
actually uses T*, not T&, as you say. But here's a patch to do that
(I hope).
> That wouldn't help with std::reference_wrapper or std::ref_view because they
> have pointer members instead of references, but perhaps loosening the check
> to include that case would make sense?
Sorry, I don't understand what you mean by loosening the check. I could
hardcode std::reference_wrapper and std::ref_view but I don't think that's
what you meant. Surely I cannot _not_ warn for any class that contains a
T*.
Here's the patch so that we have some actual code to discuss... Thanks.
-- >8 --
Here, -Wdangling-reference triggers where it probably shouldn't, causing
some grief. The code in question uses a reference wrapper with a member
function returning a reference to a subobject of a non-temporary object:
const Plane & meta = fm.planes().inner();
I've tried a few approaches, e.g., checking that the member function's
return type is the same as the type of the enclosing class (which is
the case for member functions returning *this), but that then breaks
Wdangling-reference4.C with std::optional.
Perhaps we want to look at the member function's enclosing class
to see if it's a reference wrapper class (meaning, has a reference
member and a constructor taking the same reference type) and don't
warn if so, supposing that the member function returns a reference
to a non-temporary object.
It's ugly, but better than asking users to add #pragmas into their code.
PR c++/107532
gcc/cp/ChangeLog:
* call.cc (do_warn_dangling_reference): Don't warn when the
member function comes from a reference wrapper class.
gcc/testsuite/ChangeLog:
* g++.dg/warn/Wdangling-reference8.C: New test.
---
gcc/cp/call.cc| 32
.../g++.dg/warn/Wdangling-reference8.C| 77 +++
2 files changed, 109 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference8.C
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index 0780b5840a3..b0670a21240 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -13832,6 +13832,38 @@ do_warn_dangling_reference (tree expr)
if (!(TYPE_REF_OBJ_P (rettype) || std_pair_ref_ref_p (rettype)))
return NULL_TREE;
+ /* An attempt to reduce the number of -Wdangling-reference
+ false positives concerning reference wrappers (c++/107532).
+ If the enclosing class is a reference-like class, that is, has
+ a reference member and a constructor taking the same reference type,
+ we suppose that the member function is returning a reference
+ to a non-temporary object. */
+ if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fndecl)
+ && !DECL_OVERLOADED_OPERATOR_P (fndecl))
+ {
+ tree ctx = CP_DECL_CONTEXT (fndecl);
+ for (tree fields = TYPE_FIELDS (ctx);
+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 (ctx)))
+ {
+ tree args = FUNCTION_FIRST_USER_PARMTYP
