Skip to content

feat(unused_var): analyze global variables inside function body #2944

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danmar merged 5 commits into from
Dec 19, 2020
Merged

feat(unused_var): analyze global variables inside function body #2944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a71cd03
feat(unused_var): analyze global variables inside function body
miltolstoy Dec 13, 2020
d352b15
self-check error fix
miltolstoy Dec 13, 2020
4a6018f
change std::list to std::set for pointersToGlobals
miltolstoy Dec 15, 2020
b93f9f9
treat all operations with global pointers/arrays as side-effect
miltolstoy Dec 17, 2020
14d0d32
update search in pointersToGlobals
miltolstoy Dec 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions lib/checkunusedvar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1609,17 +1609,33 @@ bool CheckUnusedVar::isFunctionWithoutSideEffects(const Function& func, const To
}

bool sideEffectReturnFound = false;
std::set<const Variable*> pointersToGlobals;
for (Token* bodyToken = func.functionScope->bodyStart->next(); bodyToken != func.functionScope->bodyEnd;
bodyToken = bodyToken->next()) {
bodyToken = bodyToken->next())
{
// check variable inside function body
const Variable* bodyVariable = bodyToken->variable();
if (bodyVariable) {
// check variable for side-effects
if (!isVariableWithoutSideEffects(*bodyVariable)) {
return false;
}
// check if global variable is changed
if (bodyVariable->isGlobal()) {
return false; // TODO: analyze global variable usage
if (bodyVariable->isGlobal() || (pointersToGlobals.find(bodyVariable) != pointersToGlobals.end()) ) {
if (bodyVariable->isPointer() || bodyVariable->isArray()) {
return false; // TODO: Update astutils.cpp:1544 isVariableChanged() and remove this. Unhandled case: `*(global_arr + 1) = new_val`
}
const int depth = 20;
if (isVariableChanged(bodyToken, depth, mSettings, mTokenizer->isCPP())) {
return false;
}
// check if pointer to global variable assigned to another variable (another_var = &global_var)
if (Token::simpleMatch(bodyToken->tokAt(-1), "&") && Token::simpleMatch(bodyToken->tokAt(-2), "=")) {
const Token* assigned_var_token = bodyToken->tokAt(-3);
if (assigned_var_token && assigned_var_token->variable())
Copy link
Owner

@danmar danmar Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this condition is true then you don't care that the address is used somehow.. would probably be safer to return false then.

Copy link
Contributor Author

@miltolstoy miltolstoy Dec 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I see, isVariableChanged() works fine enough with "pointer-to-global" variables. I prefer to leave it as is, if you not insist.
Do you see any test cases which will force this code work not as expected?

Copy link
Owner

@danmar danmar Dec 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it works well.. right now I am only worried about the todo test case. Do you know it it's complex to fix that?

Copy link
Contributor Author

@miltolstoy miltolstoy Dec 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, it should be not so complex. But it can affect existing tests and I will need some time to figure out with them.
Also I can not estimate a free time amount I will be able to put on this work in the nearest time. =)
How about this one - I will return false for now and update astutils.cpp and this piece of code in next PR?

Copy link
Owner

@danmar danmar Dec 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine to make a partial fix.. and finish later. but then I feel more comfortable if there are no false positives in the meantime. So well.. could you return false if there is a global array/pointer variable.

{
pointersToGlobals.insert(assigned_var_token->variable());
}
}
}
}

Expand Down
130 changes: 126 additions & 4 deletions test/testunusedvar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ class TestUnusedVar : public TestFixture {
"void f() {\n"
" C c;\n"
"}");
TODO_ASSERT_EQUALS("[test.cpp:11]: (style) Unused variable: c\n", "", errout.str());
ASSERT_EQUALS("[test.cpp:11]: (style) Unused variable: c\n", errout.str());

// changing global variable in return
functionVariableUsage(
Expand Down Expand Up @@ -698,6 +698,23 @@ class TestUnusedVar : public TestFixture {
"}");
ASSERT_EQUALS("", errout.str());

// global variable use in function body without change
functionVariableUsage(
"int global = 1;\n"
"int func() {\n"
" int x = global + 1;\n"
" return x;\n"
"}\n"
"class C {\n"
"public:\n"
" C() : x(func()) {}\n"
" int x;\n"
"};\n"
"void f() {\n"
" C c;\n"
"}");
ASSERT_EQUALS("[test.cpp:12]: (style) Unused variable: c\n", errout.str());

// changing global array variable in function body
functionVariableUsage(
"int x[] = {0, 1, 3};\n"
Expand All @@ -715,6 +732,38 @@ class TestUnusedVar : public TestFixture {
"}");
ASSERT_EQUALS("", errout.str());

functionVariableUsage(
"int x[] = {0, 1, 3};\n"
"int func() {\n"
" *x = 2;\n"
" return 1;\n"
"}\n"
"class C {\n"
"public:\n"
" C() : x(func()) {}\n"
" int x;\n"
"};\n"
"void f() {\n"
" C c;\n"
"}");
ASSERT_EQUALS("", errout.str());

functionVariableUsage(
"int x[] = {0, 1, 3};\n"
"int func() {\n"
" *(x + 1) = 2;\n"
" return 1;\n"
"}\n"
"class C {\n"
"public:\n"
" C() : x(func()) {}\n"
" int x;\n"
"};\n"
"void f() {\n"
" C c;\n"
"}");
ASSERT_EQUALS("", errout.str());

// changing local variable
functionVariableUsage(
"int func() {\n"
Expand Down Expand Up @@ -985,7 +1034,7 @@ class TestUnusedVar : public TestFixture {
functionVariableUsage(
"int global = 1;\n"
"int func() {\n"
" int *p = &global;\n"
" int* p = &global;\n"
" *p = 0;\n"
" return 1;\n"
"}\n"
Expand All @@ -999,11 +1048,65 @@ class TestUnusedVar : public TestFixture {
"}");
ASSERT_EQUALS("", errout.str());

// global struct variable
// global variable assigning to local pointer, but not modifying
functionVariableUsage(
"int global = 1;\n"
"int func() {\n"
" int* p = &global;\n"
" (void) p;\n"
" return 1;\n"
"}\n"
"class C {\n"
"public:\n"
" C() : x(func()) {}\n"
" int x;\n"
"};\n"
"void f() {\n"
" C c;\n"
"}");
// TODO: see TODO for global vars under CheckUnusedVar::isFunctionWithoutSideEffects()
TODO_ASSERT_EQUALS("[test.cpp:13]: (style) Unused variable: c\n", "", errout.str());

// global struct variable modification
functionVariableUsage(
"struct S { int x; } s;\n"
"int func() {\n"
" s.x = 1;\n"
" return 1;\n"
"}\n"
"class C {\n"
"public:\n"
" C() : x(func()) {}\n"
" int x;\n"
"};\n"
"void f() {\n"
" C c;\n"
"}");
ASSERT_EQUALS("", errout.str());

// global struct variable without modification
functionVariableUsage(
"struct S { int x; } s;\n"
"int func() {\n"
" s.x = 1;;\n"
" int y = s.x + 1;\n"
" return y;\n"
"}\n"
"class C {\n"
"public:\n"
" C() : x(func()) {}\n"
" int x;\n"
"};\n"
"void f() {\n"
" C c;\n"
"}");
ASSERT_EQUALS("[test.cpp:12]: (style) Unused variable: c\n", errout.str());

// global pointer to struct variable modification
functionVariableUsage(
"struct S { int x; };\n"
"struct S* s = new(struct S);\n"
"int func() {\n"
" s->x = 1;\n"
" return 1;\n"
"}\n"
"class C {\n"
Expand All @@ -1015,6 +1118,25 @@ class TestUnusedVar : public TestFixture {
" C c;\n"
"}");
ASSERT_EQUALS("", errout.str());

// global pointer to struct variable without modification
functionVariableUsage(
"struct S { int x; };\n"
"struct S* s = new(struct S);\n"
"int func() {\n"
" int y = s->x + 1;\n"
" return y;\n"
"}\n"
"class C {\n"
"public:\n"
" C() : x(func()) {}\n"
" int x;\n"
"};\n"
"void f() {\n"
" C c;\n"
"}");
// TODO: see TODO for global vars under CheckUnusedVar::isFunctionWithoutSideEffects()
TODO_ASSERT_EQUALS("[test.cpp:13]: (style) Unused variable: c\n", "", errout.str());
}

// #5355 - False positive: Variable is not assigned a value.
Expand Down