Re: [PATCH] warn for more impossible null pointer tests [PR102103]

[Jason Merrill <jason@redhat.com>]

On 9/17/21 12:02, Martin Sebor wrote:
> On 9/8/21 2:06 PM, Jason Merrill wrote:
>> On 9/2/21 7:53 PM, Martin Sebor wrote:
>>> @@ -4622,28 +4622,94 @@ warn_for_null_address (location_t location, 
>>> tree op, tsubst_flags_t complain)
>>>     if (!warn_address
>>>         || (complain & tf_warning) == 0
>>>         || c_inhibit_evaluation_warnings != 0
>>> -      || warning_suppressed_p (op, OPT_Waddress))
>>> +      || warning_suppressed_p (op, OPT_Waddress)
>>> +      || processing_template_decl != 0)
>>
>> Completely suppressing this warning in templates seems like a 
>> regression;  I'd think we could recognize many relevant cases before 
>> instantiation.  You just can't assume that ADDR_EXPR has the default 
>> meaning if it has unknown type (i.e. because op0 is type-dependent).
> 
> I added the suppression to keep g++.dg/warn/pr101219.C from failing
> but in hindsight I should have questioned the reasoning behind
> the "no warning emitted here (no instantiation)" comment in the test.
> 
> I agree that it would be helpful to diagnose the type-independent
> subset of the problem even in uninstantiated templates.  Current
> trunk doesn't (it never has), but with my patch and the suppression
> above removed it does.  I've updated the tests to expect it.
> 
> Please see the attached revision.
> 
> Martin
> 
> PS There are still more opportunities to issue -Waddress in templates
> that this patch doesn't handle, e.g.,:
> 
>    template <class T> bool f (T *p) { return &p == 0; }
> 
> Handling this will take more surgery.
> 
> PPS It seems that most other warnings (and even some errors, like
> -Wnarrowing) are suppressed in uninstantiated templates as well,
> even for non-dependent expressions.  In the few test cases I looked
> at Clang does better.  It sounds like you'd like to see improvements
> in this direction not just for -Waddress but in general.  Just for
> the avoidance of doubt, can you confirm that for future reference?

Yes, in general it's better to diagnose sooner.

> +  if (TREE_CODE (cop) == NON_LVALUE_EXPR)
> +    /* Unwrap the expression for C++ 98.  */
> +    cop = TREE_OPERAND (cop, 0);

What does this have to do with C++98?

> +  if (TREE_CODE (cop) == PTRMEM_CST)
> +    {
> +      /* The address of a nonstatic data member is never null.  */
> +      warning_at (location, OPT_Waddress,
> +		  "the address %qE will never be NULL",

Capitalizing NULL when talking about pointers-to-members seems a bit 
odd, but I guess it's fine.

The C++ changes are OK.

Jason