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. 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<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. 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 -- 2.39.0