Re: [PATCH] c++: -Wdangling-reference with reference wrapper [PR107532]

[Jason Merrill <jason@redhat.com>]

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