On 8/21/24 3:34 PM, Arsen Arsenović wrote:
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).
It seems weird to lose the "...to here" marking on the return. Any
thoughts, David?
* 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.
Sounds good.
Jason
