Message ID | 20230118175200.365397-1-polacek@redhat.com |
---|---|
State | New |
Headers | show |
Series | c++: -Wdangling-reference with reference wrapper [PR107532] | expand |
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. 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 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? Jason > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > PR c++/107532 > > gcc/cp/ChangeLog: > > * call.cc (do_warn_dangling_reference): Don't warn when the > object member functions are invoked on is not a temporary. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wdangling-reference8.C: New test. > --- > gcc/cp/call.cc | 33 +++++++- > .../g++.dg/warn/Wdangling-reference8.C | 77 +++++++++++++++++++ > 2 files changed, 109 insertions(+), 1 deletion(-) > 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..43e65c3dffb 100644 > --- a/gcc/cp/call.cc > +++ b/gcc/cp/call.cc > @@ -13850,7 +13850,38 @@ do_warn_dangling_reference (tree expr) > if (TREE_CODE (arg) == ADDR_EXPR) > arg = TREE_OPERAND (arg, 0); > if (expr_represents_temporary_p (arg)) > - return expr; > + { > + /* An ugly attempt to reduce the number of -Wdangling-reference > + false positives concerning reference wrappers (c++/107532). > + Don't warn about s.a().b() but do warn about S().a().b(), > + supposing that the member function is returning a reference > + to a subobject of the (non-temporary) object. */ > + if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fndecl) > + && !DECL_OVERLOADED_OPERATOR_P (fndecl) > + && i == 0) > + { > + tree t = arg; > + while (handled_component_p (t)) > + t = TREE_OPERAND (t, 0); > + t = TARGET_EXPR_INITIAL (arg); > + /* Quite likely we don't have a chain of member functions > + (like a().b().c()). */ > + if (TREE_CODE (t) != CALL_EXPR) > + return expr; > + /* Walk the call chain to the original object and see if > + it was a temporary. */ > + do > + t = tree_strip_nop_conversions (CALL_EXPR_ARG (t, 0)); > + while (TREE_CODE (t) == CALL_EXPR); > + /* If the object argument is &TARGET_EXPR<>, we've started > + off the chain with a temporary and we want to warn. */ > + if (TREE_CODE (t) == ADDR_EXPR) > + t = TREE_OPERAND (t, 0); > + if (!expr_represents_temporary_p (t)) > + break; > + } > + return expr; > + } > /* Don't warn about member function like: > std::any a(...); > S& s = a.emplace<S>({0}, 0); > 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..32280f3e282 > --- /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(); // { dg-warning "dangling reference" } > + (void) d8; > +} > > base-commit: c6a011119bfa038ccbfc9f123ede14a3d6237fab
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc index 0780b5840a3..43e65c3dffb 100644 --- a/gcc/cp/call.cc +++ b/gcc/cp/call.cc @@ -13850,7 +13850,38 @@ do_warn_dangling_reference (tree expr) if (TREE_CODE (arg) == ADDR_EXPR) arg = TREE_OPERAND (arg, 0); if (expr_represents_temporary_p (arg)) - return expr; + { + /* An ugly attempt to reduce the number of -Wdangling-reference + false positives concerning reference wrappers (c++/107532). + Don't warn about s.a().b() but do warn about S().a().b(), + supposing that the member function is returning a reference + to a subobject of the (non-temporary) object. */ + if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fndecl) + && !DECL_OVERLOADED_OPERATOR_P (fndecl) + && i == 0) + { + tree t = arg; + while (handled_component_p (t)) + t = TREE_OPERAND (t, 0); + t = TARGET_EXPR_INITIAL (arg); + /* Quite likely we don't have a chain of member functions + (like a().b().c()). */ + if (TREE_CODE (t) != CALL_EXPR) + return expr; + /* Walk the call chain to the original object and see if + it was a temporary. */ + do + t = tree_strip_nop_conversions (CALL_EXPR_ARG (t, 0)); + while (TREE_CODE (t) == CALL_EXPR); + /* If the object argument is &TARGET_EXPR<>, we've started + off the chain with a temporary and we want to warn. */ + if (TREE_CODE (t) == ADDR_EXPR) + t = TREE_OPERAND (t, 0); + if (!expr_represents_temporary_p (t)) + break; + } + return expr; + } /* Don't warn about member function like: std::any a(...); S& s = a.emplace<S>({0}, 0); 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..32280f3e282 --- /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(); // { dg-warning "dangling reference" } + (void) d8; +}