On 3/1/21 7:44 PM, Martin Sebor wrote:
On 3/1/21 1:33 PM, Jason Merrill wrote:
On 3/1/21 12:10 PM, Martin Sebor wrote:
On 2/24/21 8:13 PM, Jason Merrill wrote:
On 2/24/21 5:25 PM, Martin Sebor wrote:
In r11-6900 (PR 98646 - static_cast confuses -Wnonnull) we decided
that issuing -Wnonnull for dereferencing the result of dynamic_cast
was helpful despite the false positives it causes when the pointer
is guaranteed not to be null because of a prior test.
The test case in PR 99251 along with the feedback I got from Martin
Liska have convinced me it was not the right decision.
The attached patch arranges for dynamic_cast to also suppress
-Wnonnull
analogously to static_cast. Since there already is a helper function
that builds the if-not-null test (ifnonnull) and sets TREE_NO_WARNING,
I factored out the corresponding code from build_base_path that sets
the additional TREE_NO_WARNING bit for static_cast into the function
and called it from both places. I also renamed the function to make
its purpose clearer and for consistency with other build_xxx APIs.
Let's call it build_if_nonnull, as it builds the COND_EXPR as well
as the test.
Done.
+ /* The dynamic_cast might fail but so a warning might be justified
+ but not when the operand is guarded. See pr99251. */
+ if (B *q = p->bptr ())
+ dynamic_cast<C*>(q)->g (); // { dg-bogus
"\\\[-Wnonnull" }
This guard is no more necessary than it is for static_cast; both
cases deal with null arguments. Let's not add these checks to the
testcases.
Done.
This guard doesn't check for the mentioned case of dynamic_cast
failing because the B* does not in fact point to a C.
I think we can just change the dg-warning to dg-bogus. Sure,
dynamic_cast might fail, but AFAICT -Wnonnull isn't supposed to warn
about arguments that *might* be null, only arguments that are
*known* to be null.
Done.
I had added the 'if' to the test to make it clear why the warning
isn't expected. I replaced it with a comment to that effect.
FWIW, I do think a warning for dynamic_cast to a pointer would be
helpful in the absence of an if statement in these cases:
void f (B *p)
{
dynamic_cast<D*>(p)->g ();
}
Not because p may be null but because the result of the cast may be
for a nonnull p. If it's f's precondition that D is derived from B
(in addition to p being nonnull) the cast would be better written as
one to a reference to D.
Agreed, or if it isn't a precondition,
if (D* dp = dynamic_cast<D*>(p))
dp->g();
Such a warning would need to be issued by the middle end and although
it could be controlled by a new option (say -Wdynamic-cast, along with
the cases discussed in PR 12277)
Sounds good.
I don't think issuing -Wnonnull in this case would be inappropriate.
I disagree; issuing -Wnonnull for this case would be wrong. Again,
-Wnonnull is supposed to warn when an argument *is* null, not when it
*might* be null.
I think warning about this case is a good idea, just not as part of
-Wnonnull.
Anyway, that's something to consider for GCC 12. For now, please see
the revised patch.
* rtti.c (ifnonnull): Rename...
(build_nonnull_test): ...to this. Set no-warning bit on COND_EXPR.
The ChangeLog needs updating.
+ /* Unlike static_cast, dynamic cast may fail for a nonnull operand
but
Yes, but...
+ since the front end can't see if the cast is guarded against being
+ invoked with a null
No; my point was that whether the cast is guarded against being
invoked with a null is no more relevant for dynamic_cast than it is
for static_cast, as both casts give a null result for a null argument.
For this test, dynamic_cast is not significantly different from
static_cast. For both casts, the bug was the compiler warning about a
nullptr that it introduced itself.
It seems like splitting hairs. The comment (much as the original
if guard) is just meant to explain why -Wnonnull isn't expected in
case a new warning is added that detects the bad assumption above.
I want to make it clear that if/when that happens a failure here
doesn't mislead the author into thinking we don't want any warning
there at all.
I have reworded the comments yet again.
verify there's no warning. See also pr99251. */
Yes.
- dynamic_cast<const C*>(p->bptr ())->g (); // { dg-warning
"\\\[-Wnonnull" }
+ dynamic_cast<const C*>(p)->g (); // { dg-bogus
"\\\[-Wnonnull" }
Please put back the ->bptr()s.
Done.
OK, thanks.
Jason
