From 450bdfedf3fe0773fd666bfd91226211d8fb8d47 Mon Sep 17 00:00:00 2001 From: Paul Date: Mon, 13 Jul 2020 12:40:01 -0500 Subject: [PATCH 1/2] Fix FP of duplicateCondition when modifying the this variable --- lib/astutils.cpp | 31 ++++++++++++++++++++++++++- lib/astutils.h | 6 ++++++ lib/checkcondition.cpp | 6 ++++++ test/testcondition.cpp | 48 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 1 deletion(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 712847e46..7d636c829 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -539,7 +539,10 @@ bool exprDependsOnThis(const Token* expr, nonneg int depth) // calling nonstatic method? if (Token::Match(expr->previous(), "!!:: %name% (") && expr->function() && expr->function()->nestedIn && expr->function()->nestedIn->isClassOrStruct()) { // is it a method of this? - const Scope *nestedIn = expr->scope()->functionOf; + const Scope *fScope = expr->scope(); + while (!fScope->functionOf && fScope->nestedIn) + fScope = fScope->nestedIn; + const Scope *nestedIn = fScope->functionOf; if (nestedIn && nestedIn->function) nestedIn = nestedIn->function->token->scope(); while (nestedIn && nestedIn != expr->function()->nestedIn) { @@ -1562,6 +1565,32 @@ bool isVariablesChanged(const Token* start, return false; } +bool isThisChanged(const Token* start, + const Token* end, + int indirect, + const Settings* settings, + bool cpp) +{ + for (const Token* tok = start; tok != end; tok = tok->next()) { + if (!exprDependsOnThis(tok)) + continue; + if (Token::Match(tok->previous(), "%name% (")) { + if (tok->previous()->function()) { + if (!tok->previous()->function()->isConst()) + return true; + else + continue; + } else if (!tok->previous()->isKeyword()){ + return true; + } + + } + if (isVariableChanged(tok, indirect, settings, cpp)) + return true; + } + return false; +} + int numberOfArguments(const Token *start) { int arguments=0; diff --git a/lib/astutils.h b/lib/astutils.h index 3cd883f4d..20e402d42 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -192,6 +192,12 @@ bool isVariablesChanged(const Token* start, const Settings* settings, bool cpp); +bool isThisChanged(const Token* start, + const Token* end, + int indirect, + const Settings* settings, + bool cpp); + const Token* findVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); Token* findVariableChanged(Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index ec9b33c39..17e3e026e 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -453,6 +453,12 @@ void CheckCondition::duplicateCondition() bool modified = false; visitAstNodes(cond1, [&](const Token *tok3) { + if (exprDependsOnThis(tok3)) { + if (isThisChanged(scope.classDef->next(), cond2, false, mSettings, mTokenizer->isCPP())) { + modified = true; + return ChildrenToVisit::done; + } + } if (tok3->varId() > 0 && isVariableChanged(scope.classDef->next(), cond2, tok3->varId(), false, mSettings, mTokenizer->isCPP())) { modified = true; diff --git a/test/testcondition.cpp b/test/testcondition.cpp index d02abd8ad..05c6ba239 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -3668,6 +3668,54 @@ private: " if (a.b) {}\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + check("struct A {\n" + " int a;\n" + " void b() const {\n" + " return a == 1;\n" + " }\n" + " void c();\n" + " void d() {\n" + " if(b()) {\n" + " c();\n" + " }\n" + " if (b()) {\n" + " a = 3;\n" + " }\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("struct A {\n" + " int a;\n" + " void b() const {\n" + " return a == 1;\n" + " }\n" + " void d() {\n" + " if(b()) {\n" + " a = 2;\n" + " }\n" + " if (b()) {\n" + " a = 3;\n" + " }\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("struct A {\n" + " int a;\n" + " void b() const {\n" + " return a == 1;\n" + " }\n" + " void d() {\n" + " if(b()) {\n" + " }\n" + " if (b()) {\n" + " a = 3;\n" + " }\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:9]: (style) The if condition is the same as the previous if condition\n", errout.str()); } void checkInvalidTestForOverflow() { From 519f2a537a58aafffee030facfd965acf77635d5 Mon Sep 17 00:00:00 2001 From: Paul Date: Mon, 13 Jul 2020 13:55:45 -0500 Subject: [PATCH 2/2] Format --- lib/astutils.cpp | 13 ++++--------- lib/astutils.h | 6 +----- lib/checkcondition.cpp | 2 +- test/testcondition.cpp | 3 ++- 4 files changed, 8 insertions(+), 16 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 7d636c829..293ad1dcc 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -539,10 +539,10 @@ bool exprDependsOnThis(const Token* expr, nonneg int depth) // calling nonstatic method? if (Token::Match(expr->previous(), "!!:: %name% (") && expr->function() && expr->function()->nestedIn && expr->function()->nestedIn->isClassOrStruct()) { // is it a method of this? - const Scope *fScope = expr->scope(); + const Scope* fScope = expr->scope(); while (!fScope->functionOf && fScope->nestedIn) fScope = fScope->nestedIn; - const Scope *nestedIn = fScope->functionOf; + const Scope* nestedIn = fScope->functionOf; if (nestedIn && nestedIn->function) nestedIn = nestedIn->function->token->scope(); while (nestedIn && nestedIn != expr->function()->nestedIn) { @@ -1565,11 +1565,7 @@ bool isVariablesChanged(const Token* start, return false; } -bool isThisChanged(const Token* start, - const Token* end, - int indirect, - const Settings* settings, - bool cpp) +bool isThisChanged(const Token* start, const Token* end, int indirect, const Settings* settings, bool cpp) { for (const Token* tok = start; tok != end; tok = tok->next()) { if (!exprDependsOnThis(tok)) @@ -1580,10 +1576,9 @@ bool isThisChanged(const Token* start, return true; else continue; - } else if (!tok->previous()->isKeyword()){ + } else if (!tok->previous()->isKeyword()) { return true; } - } if (isVariableChanged(tok, indirect, settings, cpp)) return true; diff --git a/lib/astutils.h b/lib/astutils.h index 20e402d42..cb453482c 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -192,11 +192,7 @@ bool isVariablesChanged(const Token* start, const Settings* settings, bool cpp); -bool isThisChanged(const Token* start, - const Token* end, - int indirect, - const Settings* settings, - bool cpp); +bool isThisChanged(const Token* start, const Token* end, int indirect, const Settings* settings, bool cpp); const Token* findVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); Token* findVariableChanged(Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 17e3e026e..ca086d5b1 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -452,7 +452,7 @@ void CheckCondition::duplicateCondition() continue; bool modified = false; - visitAstNodes(cond1, [&](const Token *tok3) { + visitAstNodes(cond1, [&](const Token* tok3) { if (exprDependsOnThis(tok3)) { if (isThisChanged(scope.classDef->next(), cond2, false, mSettings, mTokenizer->isCPP())) { modified = true; diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 05c6ba239..7519a17e7 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -3715,7 +3715,8 @@ private: " }\n" " }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:9]: (style) The if condition is the same as the previous if condition\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:9]: (style) The if condition is the same as the previous if condition\n", + errout.str()); } void checkInvalidTestForOverflow() {