From 7ef714b0c697bb818f27ad951380ea78692cc02f Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sun, 13 May 2018 13:20:55 -0500 Subject: [PATCH] Fix FP with duplicate assignments by checking if the expression is unique (#1223) * Fix FP with duplicate assignments by checking if the expression is unique * Use array of pointers * Reorder scope condition --- lib/astutils.cpp | 56 +++++++++++++++++++++++++++++++++++++++++ lib/astutils.h | 2 ++ lib/checkother.cpp | 3 ++- test/testother.cpp | 63 +++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 119 insertions(+), 5 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index f6c35460b..2075a3bb5 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -28,6 +28,7 @@ #include "valueflow.h" #include +#include static bool astIsCharWithSign(const Token *tok, ValueType::Sign sign) { @@ -423,6 +424,61 @@ bool isWithoutSideEffects(bool cpp, const Token* tok) return true; } +bool isUniqueExpression(const Token* tok) +{ + if(!tok) + return true; + if(tok->function()) { + const Function * fun = tok->function(); + const Scope * scope = fun->nestedIn; + if(!scope) + return true; + for(const Function& f:scope->functionList) { + if(f.argumentList.size() == fun->argumentList.size() && f.name() != fun->name()) { + return false; + } + } + } else if(tok->variable()) { + const Variable * var = tok->variable(); + const Scope * scope = var->scope(); + if(!scope) + return true; + const Type * varType = var->type(); + // Iterate over the variables in scope and the parameters of the function if possible + const Function * fun = scope->function; + const std::list* setOfVars[] = {&scope->varlist, fun ? &fun->argumentList : nullptr}; + if (varType) { + for(const std::list* vars:setOfVars) { + if(!vars) + continue; + for(const Variable& v:*vars) { + if (v.type() && v.type()->name() == varType->name() && v.name() != var->name()) { + return false; + } + } + } + } else { + for(const std::list* vars:setOfVars) { + if(!vars) + continue; + for(const Variable& v:*vars) { + if (v.isFloatingType() == var->isFloatingType() && + v.isEnumType() == var->isEnumType() && + v.isClass() == var->isClass() && + v.isArray() == var->isArray() && + v.isPointer() == var->isPointer() && + v.name() != var->name()) + return false; + } + } + } + } else if(!isUniqueExpression(tok->astOperand1())) { + return false; + } + + return isUniqueExpression(tok->astOperand2()); +} + bool isReturnScope(const Token * const endToken) { if (!endToken || endToken->str() != "}") diff --git a/lib/astutils.h b/lib/astutils.h index 4e05c9b53..97e65bd6a 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -77,6 +77,8 @@ bool isConstExpression(const Token *tok, const Library& library, bool pure); bool isWithoutSideEffects(bool cpp, const Token* tok); +bool isUniqueExpression(const Token* tok); + /** Is scope a return scope (scope will unconditionally return) */ bool isReturnScope(const Token *endToken); diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 017075336..635ce5b90 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1947,7 +1947,8 @@ void CheckOther::checkDuplicateExpression() tok->next()->tokType() != Token::eType && tok->next()->tokType() != Token::eName && isSameExpression(_tokenizer->isCPP(), true, tok->next(), nextAssign->next(), _settings->library, true) && - isSameExpression(_tokenizer->isCPP(), true, tok->astOperand2(), nextAssign->astOperand2(), _settings->library, false)) { + isSameExpression(_tokenizer->isCPP(), true, tok->astOperand2(), nextAssign->astOperand2(), _settings->library, false) && + !isUniqueExpression(tok->astOperand2())) { duplicateAssignExpressionError(var1, var2); } } diff --git a/test/testother.cpp b/test/testother.cpp index c0261afa2..f83fb4b6a 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -133,6 +133,7 @@ private: TEST_CASE(duplicateExpressionTemplate); // #6930 TEST_CASE(oppositeExpression); TEST_CASE(duplicateVarExpression); + TEST_CASE(duplicateVarExpressionUnique); TEST_CASE(checkSignOfUnsignedVariable); TEST_CASE(checkSignOfPointer); @@ -3971,7 +3972,7 @@ private: "}"); ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); - check("struct Foo { int f() const; };\n" + check("struct Foo { int f() const; int g() const; };\n" "void test() {\n" " Foo f = Foo{};\n" " int i = f.f();\n" @@ -3979,6 +3980,15 @@ private: "}"); ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:4]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); + check("struct Foo { int f() const; int g() const; };\n" + "void test() {\n" + " Foo f = Foo{};\n" + " Foo f2 = Foo{};\n" + " int i = f.f();\n" + " int j = f.f();\n" + "}"); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:5]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); + check("int f() __attribute__((pure));\n" "void test() {\n" " int i = 1 + f();\n" @@ -3994,19 +4004,20 @@ private: ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); check("int f(int) __attribute__((pure));\n" + "int g(int) __attribute__((pure));\n" "void test() {\n" " int i = f(0);\n" " int j = f(0);\n" "}"); - ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:4]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); - check("void test(int * p) {\n" + check("void test(int * p, int * q) {\n" " int i = *p;\n" " int j = *p;\n" "}"); ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (style) Same expression used in consecutive assignments of 'i' and 'j'.\n", errout.str()); - check("struct A { int x; };" + check("struct A { int x; int y; };" "void test(A a) {\n" " int i = a.x;\n" " int j = a.x;\n" @@ -4084,6 +4095,50 @@ private: ASSERT_EQUALS("", errout.str()); } + void duplicateVarExpressionUnique() { + check("struct SW { int first; };\n" + "void foo(SW* x) {\n" + " int start = x->first;\n" + " int end = x->first;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("struct SW { int first; };\n" + "void foo(SW* x, int i, int j) {\n" + " int start = x->first;\n" + " int end = x->first;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("struct Foo { int f() const; };\n" + "void test() {\n" + " Foo f = Foo{};\n" + " int i = f.f();\n" + " int j = f.f();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void test(int * p) {\n" + " int i = *p;\n" + " int j = *p;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("struct Foo { int f() const; int g(int) const; };\n" + "void test() {\n" + " Foo f = Foo{};\n" + " int i = f.f();\n" + " int j = f.f();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("struct Foo { int f() const; };\n" + "void test() {\n" + " Foo f = Foo{};\n" + " int i = f.f();\n" + " int j = f.f();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + void checkSignOfUnsignedVariable() { check( "void foo() {\n"