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.
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
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_PARMTYPE (fn);
+ if (args
+ && same_type_p (TREE_VALUE (args), type)
+ && TREE_CHAIN (args) == void_list_node)
+ 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)
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..4d585891fae
--- /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();
+ 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();
+ (void) d2;
+ const auto& d3 = A().b.get().get().inner();
+ (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();
+ (void) d6;
+ Plane p;
+ Ref<Plane> r(p);
+ const auto& d7 = r.inner();
+ (void) d7;
+ const auto& d8 = Ref<Plane>(p).inner();
+ (void) d8;
+}
base-commit: 8e2c6e7b426b6c9c13076208b2e176d4aa1432f1
