On 2/7/23 11:46, Marek Polacek wrote:
On Sun, Feb 05, 2023 at 05:25:25PM -0800, Jason Merrill wrote:
On 1/24/23 17:49, Marek Polacek wrote:
On Fri, Jan 20, 2023 at 03:19:54PM -0500, Jason Merrill wrote:
On 1/19/23 21:03, Marek Polacek wrote:
On Thu, Jan 19, 2023 at 01:02:02PM -0500, Jason Merrill wrote:
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<std::string>.
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.
Okay, done in the patch below. Except that I'm not including a test for
std::ranges::ref_view because I don't really know how that works.
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<std::string>.
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
Done. Thanks,
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
-- >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<std::string>.
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, or is
std::reference_wrapper or std::ranges::ref_view) 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 (reference_like_class_p): New.
(do_warn_dangling_reference): Don't warn when the member function comes
from a reference_like_class_p.
gcc/testsuite/ChangeLog:
* g++.dg/warn/Wdangling-reference8.C: New test.
* g++.dg/warn/Wdangling-reference9.C: New test.
---
gcc/cp/call.cc | 48 ++++++++++++
.../g++.dg/warn/Wdangling-reference8.C | 77 +++++++++++++++++++
.../g++.dg/warn/Wdangling-reference9.C | 21 +++++
3 files changed, 146 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference8.C
create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference9.C
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index 991730713e6..672722998ee 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -13777,6 +13777,45 @@ std_pair_ref_ref_p (tree t)
return true;
}
+/* 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. */
+
+static bool
+reference_like_class_p (tree ctype)
+{
+ tree tdecl = TYPE_NAME (TYPE_MAIN_VARIANT (ctype));
+ if (decl_in_std_namespace_p (tdecl))
+ {
+ tree name = DECL_NAME (tdecl);
+ return (name
+ && (id_equal (name, "reference_wrapper")
+ || 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;
+ }
+ }
+ return false;
+}
+
/* 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
@@ -13832,6 +13871,15 @@ 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).
+ Here we suppose that a member function of such a reference
+ wrapper class returns a reference to a non-temporary object. */
+ if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fndecl)
+ && !DECL_OVERLOADED_OPERATOR_P (fndecl)
+ && reference_like_class_p (CP_DECL_CONTEXT (fndecl)))
Ah, in this case I was thinking rather than return we would want to look
through to the initializer of the reference wrapper, and warn if that's a
temporary, so we can catch the *2 cases in your tests.
So, treating ref-like classes as much like references as we can. Some of
your v1 patch ought to be useful in implementing this, but only looking
through one call at a time, not all of them like that patch.
Maybe this one, then? I still have to loop through the calls though; EXPR in
do_warn_dangling_reference can be e.g.
Ref<const Plane>::inner (&TARGET_EXPR <D.2839, FrameMetadata::planes ((const
struct FrameMetadata *) fm)>)
or
Ref<const Plane>::inner (&TARGET_EXPR <D.2908, FrameMetadata::planes (&TARGET_EXPR
<D.2898, {.p_={.bytesused=0}}>)>)
and we want to warn only about the latter, but that means that I need to
look into the nested call 'planes' to see if the initializer was a temporary.
Right, but I was thinking we want to recurse like a few lines above, rather
than loop.
Ah yes, I can do that if I introduce a parameter that tells us
if we're processing an argument or not. I think I'm finally
more or less satisfied with the patch, thanks.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
-- >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<std::string>.
This patch adjusts do_warn_dangling_reference so that we look through
reference wrapper classes (meaning, has a reference member and a
constructor taking the same reference type, or is std::reference_wrapper
or std::ranges::ref_view) and don't warn for them, supposing that the
member function returns a reference to a non-temporary object.
PR c++/107532
gcc/cp/ChangeLog:
* call.cc (reference_like_class_p): New.
(do_warn_dangling_reference): Add new bool parameter. See through
reference_like_class_p.
gcc/testsuite/ChangeLog:
* g++.dg/warn/Wdangling-reference8.C: New test.
* g++.dg/warn/Wdangling-reference9.C: New test.
---
gcc/cp/call.cc | 97 +++++++++++++++----
.../g++.dg/warn/Wdangling-reference8.C | 77 +++++++++++++++
.../g++.dg/warn/Wdangling-reference9.C | 21 ++++
3 files changed, 178 insertions(+), 17 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference8.C
create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference9.C
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index f7c5d9da94b..2a8edc2e7e2 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -13777,6 +13777,45 @@ std_pair_ref_ref_p (tree t)
return true;
}
+/* 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. */
+
+static bool
+reference_like_class_p (tree ctype)
+{
+ tree tdecl = TYPE_NAME (TYPE_MAIN_VARIANT (ctype));
+ if (decl_in_std_namespace_p (tdecl))
+ {
+ tree name = DECL_NAME (tdecl);
+ return (name
+ && (id_equal (name, "reference_wrapper")
+ || 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;
+ }
+ }
+ return false;
+}
+
/* 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
@@ -13791,12 +13830,39 @@ std_pair_ref_ref_p (tree t)
const int& y = (f(1), 42); // NULL_TREE
const int& z = f(f(1)); // f(f(1))
- EXPR is the initializer. */
+ EXPR is the initializer. If ARG_P is true, we're processing an argument
+ to a function; the point is to distinguish between, for example,
+
+ Ref::inner (&TARGET_EXPR <D.2839, F::foo (fm)>)
+
+ where we shouldn't warn, and
+
+ Ref::inner (&TARGET_EXPR <D.2908, F::foo (&TARGET_EXPR <...>)>)
+
+ where we should warn (Ref is a reference_like_class_p so we see through
+ it. */
static tree
-do_warn_dangling_reference (tree expr)
+do_warn_dangling_reference (tree expr, bool arg_p)
{
STRIP_NOPS (expr);
+ if (TREE_CODE (expr) == ADDR_EXPR)
+ expr = TREE_OPERAND (expr, 0);
I think if we move this here, we also need to check that expr before
STRIP_NOPS had REFERENCE_TYPE. OK with that change.
+ if (arg_p && expr_represents_temporary_p (expr))
+ {
+ /* An attempt to reduce the number of -Wdangling-reference
+ false positives concerning reference wrappers (c++/107532).
+ Here we suppose that a member function of such a reference
+ wrapper class returns a reference to a non-temporary object. */
+ tree e = expr;
+ while (handled_component_p (e))
+ e = TREE_OPERAND (e, 0);
+ e = TREE_TYPE (e);
+ if (!CLASS_TYPE_P (e) || !reference_like_class_p (e))
+ return expr;
+ }
+
switch (TREE_CODE (expr))
{
case CALL_EXPR:
@@ -13829,7 +13895,8 @@ do_warn_dangling_reference (tree expr)
std::pair<const int&, const int&> v = std::minmax(1, 2);
which also creates a dangling reference, because std::minmax
returns std::pair<const T&, const T&>(b, a). */
- if (!(TYPE_REF_OBJ_P (rettype) || std_pair_ref_ref_p (rettype)))
+ if (!arg_p
+ && (!(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
@@ -13842,14 +13909,10 @@ do_warn_dangling_reference (tree expr)
if (!DECL_NONSTATIC_MEMBER_FUNCTION_P (fndecl)
&& !TYPE_REF_P (TREE_TYPE (arg)))
continue;
- /* It could also be another call taking a temporary and returning
- it and initializing this reference parameter. */
- if (do_warn_dangling_reference (arg))
- return expr;
- STRIP_NOPS (arg);
- if (TREE_CODE (arg) == ADDR_EXPR)
- arg = TREE_OPERAND (arg, 0);
- if (expr_represents_temporary_p (arg))
+ /* Recurse to see if the argument is a temporary. It could also
+ be another call taking a temporary and returning it and
+ initializing this reference parameter. */
+ if (do_warn_dangling_reference (arg, /*arg_p=*/true))
return expr;
/* Don't warn about member function like:
std::any a(...);
@@ -13866,15 +13929,15 @@ do_warn_dangling_reference (tree expr)
return NULL_TREE;
}
case COMPOUND_EXPR:
- return do_warn_dangling_reference (TREE_OPERAND (expr, 1));
+ return do_warn_dangling_reference (TREE_OPERAND (expr, 1), arg_p);
case COND_EXPR:
- if (tree t = do_warn_dangling_reference (TREE_OPERAND (expr, 1)))
+ if (tree t = do_warn_dangling_reference (TREE_OPERAND (expr, 1), arg_p))
return t;
- return do_warn_dangling_reference (TREE_OPERAND (expr, 2));
+ return do_warn_dangling_reference (TREE_OPERAND (expr, 2), arg_p);
case PAREN_EXPR:
- return do_warn_dangling_reference (TREE_OPERAND (expr, 0));
+ return do_warn_dangling_reference (TREE_OPERAND (expr, 0), arg_p);
case TARGET_EXPR:
- return do_warn_dangling_reference (TARGET_EXPR_INITIAL (expr));
+ return do_warn_dangling_reference (TARGET_EXPR_INITIAL (expr), arg_p);
default:
return NULL_TREE;
}
@@ -13917,7 +13980,7 @@ maybe_warn_dangling_reference (const_tree decl, tree
init)
= make_temp_override (global_dc->dc_warn_system_headers,
(!in_system_header_at (DECL_SOURCE_LOCATION (decl))
|| global_dc->dc_warn_system_headers));
- if (tree call = do_warn_dangling_reference (init))
+ if (tree call = do_warn_dangling_reference (init, /*arg_p=*/false))
{
auto_diagnostic_group d;
if (warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wdangling_reference,
diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-reference8.C
b/gcc/testsuite/g++.dg/warn/Wdangling-reference8.C
new file mode 100644
index 00000000000..330de1fd05d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wdangling-reference8.C
@@ -0,0 +1,77 @@
+// PR c++/107532
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wdangling-reference" }
+
+struct Plane { unsigned int bytesused; };
+
+// Passes a reference through. Does not change lifetime.
+template <typename T>
+struct Ref {
+ const T& i_;
+ Ref(const T & i) : i_(i) {}
+ const T & inner();
+};
+
+struct FrameMetadata {
+ Ref<const Plane> planes() const { return p_; }
+
+ Plane p_;
+};
+
+void bar(const Plane & meta);
+void foo(const FrameMetadata & fm)
+{
+ const Plane & meta = fm.planes().inner();
+ bar(meta);
+ const Plane & meta2 = FrameMetadata().planes().inner(); // { dg-warning
"dangling reference" }
+ bar(meta2);
+}
+
+struct S {
+ const S& self () { return *this; }
+} s;
+
+const S& r1 = s.self();
+const S& r2 = S().self(); // { dg-warning "dangling reference" }
+
+struct D {
+};
+
+struct C {
+ D d;
+ Ref<const D> get() const { return d; }
+};
+
+struct B {
+ C c;
+ const C& get() const { return c; }
+ B();
+};
+
+struct A {
+ B b;
+ const B& get() const { return b; }
+};
+
+void
+g (const A& a)
+{
+ const auto& d1 = a.get().get().get().inner();
+ (void) d1;
+ const auto& d2 = A().get().get().get().inner(); // { dg-warning "dangling
reference" }
+ (void) d2;
+ const auto& d3 = A().b.get().get().inner(); // { dg-warning "dangling
reference" }
+ (void) d3;
+ const auto& d4 = a.b.get().get().inner();
+ (void) d4;
+ const auto& d5 = a.b.c.get().inner();
+ (void) d5;
+ const auto& d6 = A().b.c.get().inner(); // { dg-warning "dangling reference"
}
+ (void) d6;
+ Plane p;
+ Ref<Plane> r(p);
+ const auto& d7 = r.inner();
+ (void) d7;
+ const auto& d8 = Ref<Plane>(p).inner();
+ (void) d8;
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-reference9.C
b/gcc/testsuite/g++.dg/warn/Wdangling-reference9.C
new file mode 100644
index 00000000000..9ad83f7365e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wdangling-reference9.C
@@ -0,0 +1,21 @@
+// PR c++/107532
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wdangling-reference" }
+
+#include <functional>
+
+struct X { int n; };
+
+struct S {
+ std::reference_wrapper<const X> wrapit() const { return x; }
+ X x;
+};
+
+void
+g (const S& s)
+{
+ const auto& a1 = s.wrapit().get();
+ (void) a1;
+ const auto& a2 = S().wrapit().get(); // { dg-warning "dangling reference" }
+ (void) a2;
+}
base-commit: f661c0bb6371f355966a67b5ce71398e80792948
