Jason Merrill <ja...@redhat.com> writes: > On 8/21/24 1:52 PM, Arsen Arsenović wrote: >> For clarity, here's the entire split-up patch I intend to push, if it >> looks OK. Tested on x86_64-pc-linux-gnu. >> I've renamed the field we've discussed and also a few parameters that >> refer to 'kw' to be less specific. The code is functionally identical. >> OK for trunk? >> TIA, have a lovely day. >> ---------- >8 ---------- >> This patch improves the EXPR_LOCATION associated with parsed >> RETURN_EXPRs so that they can be used in diagnostics later. This change >> also happened to un-suppress an analyzer false-negative that was >> happening because the location of RETURN_EXPR was entirely within the >> NULL macro, which was defined in a system header. PR analyzer/116304. > > The PR number should be on its own line, and in the subject line.
It isn't a total fix for that PR, that's why I didn't name it as part of
the changelog and subject line, but to be fair it would not be wrong to
put it there anyway, as it is related. Will reword.
>> gcc/cp/ChangeLog:
>> * cp-tree.h (finish_return_stmt): Add optional location_t
>> parameter, defaulting to input_location.
>> * parser.cc (cp_parser_jump_statement): Improve return and
>> co_return locations so that they span their entire statements.
>> * semantics.cc (finish_return_stmt): Use the new stmt_loc
>> parameter in place of input_location.
>> gcc/testsuite/ChangeLog:
>> * c-c++-common/analyzer/inlining-4-multiline.c: Adjust locations
>> in diagnostics.
>
> This doesn't look like a location adjustment, but removing testing of expected
> output. If the output changed, please change the test to check the new output
> rather than not at all.
This is the new output - the diagnostics no longer expand that macro,
since the location is not wholly contained within it. The relevant part
of the diagnostics after the change is:
--8<---------------cut here---------------start------------->8---
|
'const char* inner(int)': event 5 (depth 3)
|
| return NULL;
|
--8<---------------cut here---------------end--------------->8---
... as opposed to (excuse the quote difference - the former was pulled
from g++.log and the latter from a manual invocation):
--8<---------------cut here---------------start------------->8---
|
‘const char* inner(int)’: event 5 (depth 3)
|
|/home/arsen/gcc-mine/gcc/testsuite/c-c++-common/analyzer/../../gcc.dg/analyzer/analyzer-decls.h:7:14:
| #define NULL nullptr
| ^~~~~~~
| |
| (5) ...to here
/home/arsen/gcc-mine/gcc/testsuite/c-c++-common/analyzer/inlining-4-multiline.c:14:12:
note: in expansion of macro ‘NULL’
| return NULL;
| ^~~~
|
--8<---------------cut here---------------end--------------->8---
... presumably, the diagnostics chose to elide those bits of output due
to the new location covering the entire line (and hence not being too
informative) - but I haven't debugged that (as I assumed the diagnostic
code is DTRT now).
>> * c-c++-common/analyzer/malloc-paths-9-noexcept.c: Ditto.
>
> ...like you do properly in this test.
>
>> * c-c++-common/analyzer/malloc-CWE-401-example.c: Accept the
>> new warning on line 34 (fixed false negative).
>
> I'd think the new dg-warning could replace the obsolete TODO?
I left it because the TODO still exists for C (which wasn't fixed in
this patch - hence the { target c++ }). I can clarify it like:
/* TODO: should complain that "buf" is leaked on this path in C
also. */
... which should be more informative.
--
Arsen Arsenović
signature.asc
Description: PGP signature
