From a71cd03e3b27b54322dd40cc0c4882475a6ae9c2 Mon Sep 17 00:00:00 2001 From: miltolstoy Date: Sun, 13 Dec 2020 11:04:00 +0200 Subject: [PATCH 1/5] feat(unused_var): analyze global variables inside function body --- lib/checkunusedvar.cpp | 22 +++++-- test/testunusedvar.cpp | 129 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 143 insertions(+), 8 deletions(-) diff --git a/lib/checkunusedvar.cpp b/lib/checkunusedvar.cpp index e20b87d2c4d..626c3f7bc8e 100644 --- a/lib/checkunusedvar.cpp +++ b/lib/checkunusedvar.cpp @@ -1609,17 +1609,31 @@ bool CheckUnusedVar::isFunctionWithoutSideEffects(const Function& func, const To } bool sideEffectReturnFound = false; + std::list 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() || + (std::find(pointersToGlobals.begin(), pointersToGlobals.end(), bodyVariable) != pointersToGlobals.end())) + { + if (isVariableChanged(bodyToken, 20, nullptr, true)) { + return false; + } + // check if pointer to global variable assigned to another variable (another_var = &global_var) + if (Token::simpleMatch(bodyToken->previous(), "&") && Token::simpleMatch(bodyToken->previous()->previous(), "=")) { + const Token* assigned_var_token = bodyToken->previous()->previous()->previous(); + if (assigned_var_token && assigned_var_token->variable()) + { + pointersToGlobals.push_back(assigned_var_token->variable()); + } + } } } diff --git a/test/testunusedvar.cpp b/test/testunusedvar.cpp index af2eb67827d..b82883980a7 100644 --- a/test/testunusedvar.cpp +++ b/test/testunusedvar.cpp @@ -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( @@ -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" @@ -715,6 +732,39 @@ 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" + "}"); + // TODO: update astutils.cpp:1544 isVariableChanged() - array change through pointer arithmetic is not handled + TODO_ASSERT_EQUALS("", "[test.cpp:12]: (style) Unused variable: c\n", errout.str()); + // changing local variable functionVariableUsage( "int func() {\n" @@ -985,7 +1035,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" @@ -999,11 +1049,64 @@ 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" + "}"); + 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" + " 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" + " 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" @@ -1015,6 +1118,24 @@ 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" + "}"); + ASSERT_EQUALS("[test.cpp:13]: (style) Unused variable: c\n", errout.str()); } // #5355 - False positive: Variable is not assigned a value. From d352b158fde7f06ccdff1e430698abf95e2ab850 Mon Sep 17 00:00:00 2001 From: miltolstoy Date: Sun, 13 Dec 2020 11:46:57 +0200 Subject: [PATCH 2/5] self-check error fix --- lib/checkunusedvar.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/checkunusedvar.cpp b/lib/checkunusedvar.cpp index 626c3f7bc8e..878c4fada47 100644 --- a/lib/checkunusedvar.cpp +++ b/lib/checkunusedvar.cpp @@ -1627,8 +1627,8 @@ bool CheckUnusedVar::isFunctionWithoutSideEffects(const Function& func, const To return false; } // check if pointer to global variable assigned to another variable (another_var = &global_var) - if (Token::simpleMatch(bodyToken->previous(), "&") && Token::simpleMatch(bodyToken->previous()->previous(), "=")) { - const Token* assigned_var_token = bodyToken->previous()->previous()->previous(); + 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()) { pointersToGlobals.push_back(assigned_var_token->variable()); From 4a6018faa51acfa2d3008ddba8cf29de3d326522 Mon Sep 17 00:00:00 2001 From: miltolstoy Date: Tue, 15 Dec 2020 18:15:29 +0200 Subject: [PATCH 3/5] change std::list to std::set for pointersToGlobals --- lib/checkunusedvar.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/checkunusedvar.cpp b/lib/checkunusedvar.cpp index 878c4fada47..7e0c289ca08 100644 --- a/lib/checkunusedvar.cpp +++ b/lib/checkunusedvar.cpp @@ -1609,7 +1609,7 @@ bool CheckUnusedVar::isFunctionWithoutSideEffects(const Function& func, const To } bool sideEffectReturnFound = false; - std::list pointersToGlobals; + std::set pointersToGlobals; for (Token* bodyToken = func.functionScope->bodyStart->next(); bodyToken != func.functionScope->bodyEnd; bodyToken = bodyToken->next()) { @@ -1623,7 +1623,8 @@ bool CheckUnusedVar::isFunctionWithoutSideEffects(const Function& func, const To if (bodyVariable->isGlobal() || (std::find(pointersToGlobals.begin(), pointersToGlobals.end(), bodyVariable) != pointersToGlobals.end())) { - if (isVariableChanged(bodyToken, 20, nullptr, true)) { + 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) @@ -1631,7 +1632,7 @@ bool CheckUnusedVar::isFunctionWithoutSideEffects(const Function& func, const To const Token* assigned_var_token = bodyToken->tokAt(-3); if (assigned_var_token && assigned_var_token->variable()) { - pointersToGlobals.push_back(assigned_var_token->variable()); + pointersToGlobals.insert(assigned_var_token->variable()); } } } From b93f9f945ba0e4c94954a7575886f9ee6fe9d4ee Mon Sep 17 00:00:00 2001 From: miltolstoy Date: Thu, 17 Dec 2020 18:43:11 +0200 Subject: [PATCH 4/5] treat all operations with global pointers/arrays as side-effect --- lib/checkunusedvar.cpp | 3 +++ test/testunusedvar.cpp | 9 +++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/checkunusedvar.cpp b/lib/checkunusedvar.cpp index 7e0c289ca08..c35661c7066 100644 --- a/lib/checkunusedvar.cpp +++ b/lib/checkunusedvar.cpp @@ -1623,6 +1623,9 @@ bool CheckUnusedVar::isFunctionWithoutSideEffects(const Function& func, const To if (bodyVariable->isGlobal() || (std::find(pointersToGlobals.begin(), pointersToGlobals.end(), 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; diff --git a/test/testunusedvar.cpp b/test/testunusedvar.cpp index b82883980a7..dd240c5b1ce 100644 --- a/test/testunusedvar.cpp +++ b/test/testunusedvar.cpp @@ -762,8 +762,7 @@ class TestUnusedVar : public TestFixture { "void f() {\n" " C c;\n" "}"); - // TODO: update astutils.cpp:1544 isVariableChanged() - array change through pointer arithmetic is not handled - TODO_ASSERT_EQUALS("", "[test.cpp:12]: (style) Unused variable: c\n", errout.str()); + ASSERT_EQUALS("", errout.str()); // changing local variable functionVariableUsage( @@ -1065,7 +1064,8 @@ class TestUnusedVar : public TestFixture { "void f() {\n" " C c;\n" "}"); - ASSERT_EQUALS("[test.cpp:13]: (style) Unused variable: c\n", errout.str()); + // 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( @@ -1135,7 +1135,8 @@ class TestUnusedVar : public TestFixture { "void f() {\n" " C c;\n" "}"); - ASSERT_EQUALS("[test.cpp:13]: (style) Unused variable: c\n", errout.str()); + // 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. From 14d0d328dea18607dc3c9aec3bec8c9226047ad7 Mon Sep 17 00:00:00 2001 From: miltolstoy Date: Thu, 17 Dec 2020 20:23:05 +0200 Subject: [PATCH 5/5] update search in pointersToGlobals --- lib/checkunusedvar.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/checkunusedvar.cpp b/lib/checkunusedvar.cpp index c35661c7066..7c41a9798c8 100644 --- a/lib/checkunusedvar.cpp +++ b/lib/checkunusedvar.cpp @@ -1620,9 +1620,7 @@ bool CheckUnusedVar::isFunctionWithoutSideEffects(const Function& func, const To return false; } // check if global variable is changed - if (bodyVariable->isGlobal() || - (std::find(pointersToGlobals.begin(), pointersToGlobals.end(), bodyVariable) != pointersToGlobals.end())) - { + 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` }