diff mbox series

[v2] c++: suppress -Waddress warnings with *_cast [PR105569]

Message ID Yo6Qyrklo3DwgmHY@redhat.com
State New
Headers show
Series [v2] c++: suppress -Waddress warnings with *_cast [PR105569] | expand

Commit Message

Marek Polacek May 25, 2022, 8:25 p.m. UTC 
On Wed, May 18, 2022 at 09:43:47AM -0400, Jason Merrill wrote:
> On 5/16/22 13:06, Marek Polacek wrote:
> > dynamic_cast can legally return nullptr, so I don't think it's helpful
> > for -Waddress to warn for
> > 
> >    if (dynamic_cast<A*>(&ref))
> >      // ...
> > 
> > More generally, it's likely not useful to warn for the artificial
> > POINTER_PLUS_EXPRs created by build_base_path.
> 
> Normally the POINTER_PLUS_EXPR is guarded by if (nonnull).  But
> build_base_path isn't adding that guard in this case because the operand is
> known to be a reference, which cannot be null
> (http://eel.is/c++draft/dcl.ref#5).  So a warning is indicated for this
> testcase, though it would be good to give a more informative one ("comparing
> address of reference to null").

Ah, got it.  How about this patch instead?  Thanks,

-- >8 --
This patch improves the diagnostic for -Waddress when it warns for

  if (dynamic_cast<A*>(&ref))
    // ...

where 'ref' is a reference, which cannot be null.  In particular, it
changes
warning: comparing the result of pointer addition '(((A*)ref) + ((sizetype)(*(long int*)((& ref)->B::_vptr.B + -24))))' and NULL
to
warning: comparing address of reference 'ref' to null

	PR c++/105569

gcc/cp/ChangeLog:

	* typeck.cc (warn_for_null_address): Improve the warning when
	the POINTER_PLUS_EXPR's base is of reference type.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Waddress-9.C: New test.
---
 gcc/cp/typeck.cc                       | 12 ++++++++--
 gcc/testsuite/g++.dg/warn/Waddress-9.C | 31 ++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Waddress-9.C


base-commit: 34970d08c6297e12f3f9117b6ac19fb2de522e24

Comments

Jason Merrill May 26, 2022, 1:26 p.m. UTC | #1 
On 5/25/22 16:25, Marek Polacek wrote:
> On Wed, May 18, 2022 at 09:43:47AM -0400, Jason Merrill wrote:
>> On 5/16/22 13:06, Marek Polacek wrote:
>>> dynamic_cast can legally return nullptr, so I don't think it's helpful
>>> for -Waddress to warn for
>>>
>>>     if (dynamic_cast<A*>(&ref))
>>>       // ...
>>>
>>> More generally, it's likely not useful to warn for the artificial
>>> POINTER_PLUS_EXPRs created by build_base_path.
>>
>> Normally the POINTER_PLUS_EXPR is guarded by if (nonnull).  But
>> build_base_path isn't adding that guard in this case because the operand is
>> known to be a reference, which cannot be null
>> (http://eel.is/c++draft/dcl.ref#5).  So a warning is indicated for this
>> testcase, though it would be good to give a more informative one ("comparing
>> address of reference to null").
> 
> Ah, got it.  How about this patch instead?  Thanks,

Do we also warn about plain &ref == nullptr?

> -- >8 --
> This patch improves the diagnostic for -Waddress when it warns for
> 
>    if (dynamic_cast<A*>(&ref))
>      // ...
> 
> where 'ref' is a reference, which cannot be null.  In particular, it
> changes
> warning: comparing the result of pointer addition '(((A*)ref) + ((sizetype)(*(long int*)((& ref)->B::_vptr.B + -24))))' and NULL
> to
> warning: comparing address of reference 'ref' to null
> 
> 	PR c++/105569
> 
> gcc/cp/ChangeLog:
> 
> 	* typeck.cc (warn_for_null_address): Improve the warning when
> 	the POINTER_PLUS_EXPR's base is of reference type.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/warn/Waddress-9.C: New test.
> ---
>   gcc/cp/typeck.cc                       | 12 ++++++++--
>   gcc/testsuite/g++.dg/warn/Waddress-9.C | 31 ++++++++++++++++++++++++++
>   2 files changed, 41 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/warn/Waddress-9.C
> 
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index 385cdf4d733..0837f2484ba 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -4757,8 +4757,16 @@ warn_for_null_address (location_t location, tree op, tsubst_flags_t complain)
>         tree off = TREE_OPERAND (cop, 1);
>         if (!integer_zerop (off)
>   	  && !warning_suppressed_p (cop, OPT_Waddress))
> -	warning_at (location, OPT_Waddress, "comparing the result of pointer "
> -		    "addition %qE and NULL", cop);
> +	{
> +	  tree base = TREE_OPERAND (cop, 0);
> +	  STRIP_NOPS (base);
> +	  if (TYPE_REF_P (TREE_TYPE (base)))
> +	    warning_at (location, OPT_Waddress, "comparing address of "
> +			"reference %qE and NULL", base);
> +	  else
> +	    warning_at (location, OPT_Waddress, "comparing the result of "
> +			"pointer addition %qE and NULL", cop);
> +	}
>         return;
>       }
>     else if (CONVERT_EXPR_P (op)
> diff --git a/gcc/testsuite/g++.dg/warn/Waddress-9.C b/gcc/testsuite/g++.dg/warn/Waddress-9.C
> new file mode 100644
> index 00000000000..a3654ff1f91
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Waddress-9.C
> @@ -0,0 +1,31 @@
> +// PR c++/105569
> +// { dg-do compile { target c++11 } }
> +// { dg-options -Waddress }
> +
> +class A {};
> +
> +class B : public virtual A {};
> +
> +class C : public A {};
> +
> +int main() {
> +    B* object = new B();
> +    B &ref = *object;
> +
> +    bool b = nullptr == dynamic_cast<A*>(&ref); // { dg-warning "comparing address of reference .ref. and NULL" }
> +    bool b4 = nullptr == static_cast<A*>(&ref); // { dg-warning "comparing address of reference .ref. and NULL" }
> +    if (dynamic_cast<A*>(&ref)) // { dg-warning "comparing address of reference .ref. and NULL" }
> +      {
> +      }
> +    if (static_cast<A*>(&ref)) // { dg-warning "comparing address of reference .ref. and NULL" }
> +      {
> +      }
> +
> +    auto ptr = dynamic_cast<A*>(&ref);
> +    bool b2 = ptr == nullptr;
> +
> +    C* cobject = new C();
> +    C &cref = *cobject;
> +
> +    bool b3 = nullptr == dynamic_cast<A*>(&cref);
> +}
> 
> base-commit: 34970d08c6297e12f3f9117b6ac19fb2de522e24
Marek Polacek May 26, 2022, 1:33 p.m. UTC | #2 
On Thu, May 26, 2022 at 09:26:16AM -0400, Jason Merrill wrote:
> On 5/25/22 16:25, Marek Polacek wrote:
> > On Wed, May 18, 2022 at 09:43:47AM -0400, Jason Merrill wrote:
> > > On 5/16/22 13:06, Marek Polacek wrote:
> > > > dynamic_cast can legally return nullptr, so I don't think it's helpful
> > > > for -Waddress to warn for
> > > > 
> > > >     if (dynamic_cast<A*>(&ref))
> > > >       // ...
> > > > 
> > > > More generally, it's likely not useful to warn for the artificial
> > > > POINTER_PLUS_EXPRs created by build_base_path.
> > > 
> > > Normally the POINTER_PLUS_EXPR is guarded by if (nonnull).  But
> > > build_base_path isn't adding that guard in this case because the operand is
> > > known to be a reference, which cannot be null
> > > (http://eel.is/c++draft/dcl.ref#5).  So a warning is indicated for this
> > > testcase, though it would be good to give a more informative one ("comparing
> > > address of reference to null").
> > 
> > Ah, got it.  How about this patch instead?  Thanks,
> 
> Do we also warn about plain &ref == nullptr?

Yeah, with different wording:

p.C:5:12: warning: the compiler can assume that the address of ‘ref’ will never be NULL [-Waddress]
    5 |   if (&ref == nullptr)
      |       ~~~~~^~~~~~~~~~

warn/Walways-true-3.C tests this.

> > -- >8 --
> > This patch improves the diagnostic for -Waddress when it warns for
> > 
> >    if (dynamic_cast<A*>(&ref))
> >      // ...
> > 
> > where 'ref' is a reference, which cannot be null.  In particular, it
> > changes
> > warning: comparing the result of pointer addition '(((A*)ref) + ((sizetype)(*(long int*)((& ref)->B::_vptr.B + -24))))' and NULL
> > to
> > warning: comparing address of reference 'ref' to null
> > 
> > 	PR c++/105569
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* typeck.cc (warn_for_null_address): Improve the warning when
> > 	the POINTER_PLUS_EXPR's base is of reference type.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/warn/Waddress-9.C: New test.
> > ---
> >   gcc/cp/typeck.cc                       | 12 ++++++++--
> >   gcc/testsuite/g++.dg/warn/Waddress-9.C | 31 ++++++++++++++++++++++++++
> >   2 files changed, 41 insertions(+), 2 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/warn/Waddress-9.C
> > 
> > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> > index 385cdf4d733..0837f2484ba 100644
> > --- a/gcc/cp/typeck.cc
> > +++ b/gcc/cp/typeck.cc
> > @@ -4757,8 +4757,16 @@ warn_for_null_address (location_t location, tree op, tsubst_flags_t complain)
> >         tree off = TREE_OPERAND (cop, 1);
> >         if (!integer_zerop (off)
> >   	  && !warning_suppressed_p (cop, OPT_Waddress))
> > -	warning_at (location, OPT_Waddress, "comparing the result of pointer "
> > -		    "addition %qE and NULL", cop);
> > +	{
> > +	  tree base = TREE_OPERAND (cop, 0);
> > +	  STRIP_NOPS (base);
> > +	  if (TYPE_REF_P (TREE_TYPE (base)))
> > +	    warning_at (location, OPT_Waddress, "comparing address of "
> > +			"reference %qE and NULL", base);
> > +	  else
> > +	    warning_at (location, OPT_Waddress, "comparing the result of "
> > +			"pointer addition %qE and NULL", cop);
> > +	}
> >         return;
> >       }
> >     else if (CONVERT_EXPR_P (op)
> > diff --git a/gcc/testsuite/g++.dg/warn/Waddress-9.C b/gcc/testsuite/g++.dg/warn/Waddress-9.C
> > new file mode 100644
> > index 00000000000..a3654ff1f91
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/warn/Waddress-9.C
> > @@ -0,0 +1,31 @@
> > +// PR c++/105569
> > +// { dg-do compile { target c++11 } }
> > +// { dg-options -Waddress }
> > +
> > +class A {};
> > +
> > +class B : public virtual A {};
> > +
> > +class C : public A {};
> > +
> > +int main() {
> > +    B* object = new B();
> > +    B &ref = *object;
> > +
> > +    bool b = nullptr == dynamic_cast<A*>(&ref); // { dg-warning "comparing address of reference .ref. and NULL" }
> > +    bool b4 = nullptr == static_cast<A*>(&ref); // { dg-warning "comparing address of reference .ref. and NULL" }
> > +    if (dynamic_cast<A*>(&ref)) // { dg-warning "comparing address of reference .ref. and NULL" }
> > +      {
> > +      }
> > +    if (static_cast<A*>(&ref)) // { dg-warning "comparing address of reference .ref. and NULL" }
> > +      {
> > +      }
> > +
> > +    auto ptr = dynamic_cast<A*>(&ref);
> > +    bool b2 = ptr == nullptr;
> > +
> > +    C* cobject = new C();
> > +    C &cref = *cobject;
> > +
> > +    bool b3 = nullptr == dynamic_cast<A*>(&cref);
> > +}
> > 
> > base-commit: 34970d08c6297e12f3f9117b6ac19fb2de522e24
> 

Marek
Jason Merrill May 26, 2022, 8:34 p.m. UTC | #3 
On 5/26/22 09:33, Marek Polacek wrote:
> On Thu, May 26, 2022 at 09:26:16AM -0400, Jason Merrill wrote:
>> On 5/25/22 16:25, Marek Polacek wrote:
>>> On Wed, May 18, 2022 at 09:43:47AM -0400, Jason Merrill wrote:
>>>> On 5/16/22 13:06, Marek Polacek wrote:
>>>>> dynamic_cast can legally return nullptr, so I don't think it's helpful
>>>>> for -Waddress to warn for
>>>>>
>>>>>      if (dynamic_cast<A*>(&ref))
>>>>>        // ...
>>>>>
>>>>> More generally, it's likely not useful to warn for the artificial
>>>>> POINTER_PLUS_EXPRs created by build_base_path.
>>>>
>>>> Normally the POINTER_PLUS_EXPR is guarded by if (nonnull).  But
>>>> build_base_path isn't adding that guard in this case because the operand is
>>>> known to be a reference, which cannot be null
>>>> (http://eel.is/c++draft/dcl.ref#5).  So a warning is indicated for this
>>>> testcase, though it would be good to give a more informative one ("comparing
>>>> address of reference to null").
>>>
>>> Ah, got it.  How about this patch instead?  Thanks,
>>
>> Do we also warn about plain &ref == nullptr?
> 
> Yeah, with different wording:
> 
> p.C:5:12: warning: the compiler can assume that the address of ‘ref’ will never be NULL [-Waddress]
>      5 |   if (&ref == nullptr)
>        |       ~~~~~^~~~~~~~~~
> 
> warn/Walways-true-3.C tests this.

Let's use that wording in this case as well, then.  OK with that change.

>>> -- >8 --
>>> This patch improves the diagnostic for -Waddress when it warns for
>>>
>>>     if (dynamic_cast<A*>(&ref))
>>>       // ...
>>>
>>> where 'ref' is a reference, which cannot be null.  In particular, it
>>> changes
>>> warning: comparing the result of pointer addition '(((A*)ref) + ((sizetype)(*(long int*)((& ref)->B::_vptr.B + -24))))' and NULL
>>> to
>>> warning: comparing address of reference 'ref' to null
>>>
>>> 	PR c++/105569
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* typeck.cc (warn_for_null_address): Improve the warning when
>>> 	the POINTER_PLUS_EXPR's base is of reference type.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/warn/Waddress-9.C: New test.
>>> ---
>>>    gcc/cp/typeck.cc                       | 12 ++++++++--
>>>    gcc/testsuite/g++.dg/warn/Waddress-9.C | 31 ++++++++++++++++++++++++++
>>>    2 files changed, 41 insertions(+), 2 deletions(-)
>>>    create mode 100644 gcc/testsuite/g++.dg/warn/Waddress-9.C
>>>
>>> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
>>> index 385cdf4d733..0837f2484ba 100644
>>> --- a/gcc/cp/typeck.cc
>>> +++ b/gcc/cp/typeck.cc
>>> @@ -4757,8 +4757,16 @@ warn_for_null_address (location_t location, tree op, tsubst_flags_t complain)
>>>          tree off = TREE_OPERAND (cop, 1);
>>>          if (!integer_zerop (off)
>>>    	  && !warning_suppressed_p (cop, OPT_Waddress))
>>> -	warning_at (location, OPT_Waddress, "comparing the result of pointer "
>>> -		    "addition %qE and NULL", cop);
>>> +	{
>>> +	  tree base = TREE_OPERAND (cop, 0);
>>> +	  STRIP_NOPS (base);
>>> +	  if (TYPE_REF_P (TREE_TYPE (base)))
>>> +	    warning_at (location, OPT_Waddress, "comparing address of "
>>> +			"reference %qE and NULL", base);
>>> +	  else
>>> +	    warning_at (location, OPT_Waddress, "comparing the result of "
>>> +			"pointer addition %qE and NULL", cop);
>>> +	}
>>>          return;
>>>        }
>>>      else if (CONVERT_EXPR_P (op)
>>> diff --git a/gcc/testsuite/g++.dg/warn/Waddress-9.C b/gcc/testsuite/g++.dg/warn/Waddress-9.C
>>> new file mode 100644
>>> index 00000000000..a3654ff1f91
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/warn/Waddress-9.C
>>> @@ -0,0 +1,31 @@
>>> +// PR c++/105569
>>> +// { dg-do compile { target c++11 } }
>>> +// { dg-options -Waddress }
>>> +
>>> +class A {};
>>> +
>>> +class B : public virtual A {};
>>> +
>>> +class C : public A {};
>>> +
>>> +int main() {
>>> +    B* object = new B();
>>> +    B &ref = *object;
>>> +
>>> +    bool b = nullptr == dynamic_cast<A*>(&ref); // { dg-warning "comparing address of reference .ref. and NULL" }
>>> +    bool b4 = nullptr == static_cast<A*>(&ref); // { dg-warning "comparing address of reference .ref. and NULL" }
>>> +    if (dynamic_cast<A*>(&ref)) // { dg-warning "comparing address of reference .ref. and NULL" }
>>> +      {
>>> +      }
>>> +    if (static_cast<A*>(&ref)) // { dg-warning "comparing address of reference .ref. and NULL" }
>>> +      {
>>> +      }
>>> +
>>> +    auto ptr = dynamic_cast<A*>(&ref);
>>> +    bool b2 = ptr == nullptr;
>>> +
>>> +    C* cobject = new C();
>>> +    C &cref = *cobject;
>>> +
>>> +    bool b3 = nullptr == dynamic_cast<A*>(&cref);
>>> +}
>>>
>>> base-commit: 34970d08c6297e12f3f9117b6ac19fb2de522e24
>>
> 
> Marek
>
diff mbox series

Patch

diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index 385cdf4d733..0837f2484ba 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -4757,8 +4757,16 @@  warn_for_null_address (location_t location, tree op, tsubst_flags_t complain)
       tree off = TREE_OPERAND (cop, 1);
       if (!integer_zerop (off)
 	  && !warning_suppressed_p (cop, OPT_Waddress))
-	warning_at (location, OPT_Waddress, "comparing the result of pointer "
-		    "addition %qE and NULL", cop);
+	{
+	  tree base = TREE_OPERAND (cop, 0);
+	  STRIP_NOPS (base);
+	  if (TYPE_REF_P (TREE_TYPE (base)))
+	    warning_at (location, OPT_Waddress, "comparing address of "
+			"reference %qE and NULL", base);
+	  else
+	    warning_at (location, OPT_Waddress, "comparing the result of "
+			"pointer addition %qE and NULL", cop);
+	}
       return;
     }
   else if (CONVERT_EXPR_P (op)
diff --git a/gcc/testsuite/g++.dg/warn/Waddress-9.C b/gcc/testsuite/g++.dg/warn/Waddress-9.C
new file mode 100644
index 00000000000..a3654ff1f91
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Waddress-9.C
@@ -0,0 +1,31 @@ 
+// PR c++/105569
+// { dg-do compile { target c++11 } }
+// { dg-options -Waddress }
+
+class A {};
+
+class B : public virtual A {};
+
+class C : public A {};
+
+int main() {
+    B* object = new B();
+    B &ref = *object;
+
+    bool b = nullptr == dynamic_cast<A*>(&ref); // { dg-warning "comparing address of reference .ref. and NULL" }
+    bool b4 = nullptr == static_cast<A*>(&ref); // { dg-warning "comparing address of reference .ref. and NULL" }
+    if (dynamic_cast<A*>(&ref)) // { dg-warning "comparing address of reference .ref. and NULL" }
+      {
+      }
+    if (static_cast<A*>(&ref)) // { dg-warning "comparing address of reference .ref. and NULL" }
+      {
+      }
+
+    auto ptr = dynamic_cast<A*>(&ref);
+    bool b2 = ptr == nullptr;
+
+    C* cobject = new C();
+    C &cref = *cobject;
+
+    bool b3 = nullptr == dynamic_cast<A*>(&cref);
+}