Repository navigation
Fix #15114 FP shadowVariable (global variable prior to declaration) - #8947
chrchr-github wants to merge 2 commits into
Conversation
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The new check compares line numbers without looking at which file each token is in, so its result depends on where headers are included. A global declared in an included header is now skipped whenever its line number in the header is larger than the line of the local variable in the .c file. That is a new false negative: base reports it and the PR does not. The opposite layout is still a false positive: a function in a header shadows a global that is defined later in the .c file. findShadowed already uses precedes(f.tokenDef, var.nameToken()) for functions, which compares token order. Using precedes() for the global-scope variables too would fix both cases and also the single-line case (int f() { int i = 0; return i; } int i = 1; still warns). A smaller gap: the same false positive remains when the variable is defined later inside a namespace scope.
Testing
I built cppcheck and testrunner in pr/ and cppcheck in base/. ./testrunner TestOther passed: 252 tests, 0 failures.
New false negative (base warns, the PR does not):
# h/a.h: 9 empty lines, then on line 10:
int i = 1;
# t1.c
#include "a.h"
int f() {
int i = 0;
return i;
}
cppcheck -q --enable=style -Ih t1.c
- base:
t1.c:3:9: style: Local variable 'i' shadows outer variable [shadowVariable](note:h/a.h:10:5: Shadowed variable) - PR: no output
False positive that remains (the PR still warns, although the global comes after the function):
# h/b.h: 9 empty lines, then
int f() {
int i = 0;
return i;
}
# t5.c
#include "b.h"
int i = 1;
The PR prints h/b.h:11:9: style: Local variable 'i' shadows outer variable [shadowVariable] (note: t5.c:2:5: Shadowed variable).
Other cases I ran with the PR:
int f() { int i = 0; return i; } int i = 1;on one line still warns, because both lines are equal.- The same code as the PR's test, but inside
namespace N { ... }, still warns. extern int i;before the function still warns, which is correct.
Automated review of 007ab4b by Claude. The pull request was built and tested locally.
| return nullptr; | ||
| for (const Variable &v : scope->varlist) { | ||
| if (scope->isExecutable() && v.nameToken()->linenr() > linenr) | ||
| if ((scope->isExecutable() || scope->type == ScopeType::eGlobal) && v.nameToken()->linenr() > linenr) |
There was a problem hiding this comment.
AI review
linenr() is compared without the file index. A global from an included header with a higher line number is now skipped, which is a new false negative compared with base. A function in a header that shadows a later global in the .c file still warns. The same-line case still warns too. For the global-scope case, consider !precedes(v.nameToken(), var.nameToken()), which the function lookup below already uses. eNamespace scopes have the same issue: namespace N { int f(){int i=0;return i;} int i=1; } still warns.
No description provided.