From 5d369f50240b10a1e8a3eec15728933b999c1175 Mon Sep 17 00:00:00 2001 From: alonlib12 Date: Tue, 26 Jan 2021 22:18:18 +0200 Subject: [PATCH 1/5] Fix CheckClass::operatorEqToSelf --- lib/checkclass.cpp | 76 +++++++++++++++++++++---- lib/checkclass.h | 6 +- test/testclass.cpp | 139 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 208 insertions(+), 13 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index d0c464d49f5..810893ff6fd 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1505,27 +1505,41 @@ void CheckClass::operatorEqToSelf() if (Token::Match(func.retDef, "%type% &") && func.retDef->str() == scope->className) { // find the parameter name const Token *rhs = func.argumentList.begin()->nameToken(); - - if (!hasAssignSelf(&func, rhs)) { + const Token* out_ifStatementScopeStart = nullptr; + if (!hasAssignSelf(&func, rhs, &out_ifStatementScopeStart)) { if (hasAllocation(&func, scope)) operatorEqToSelfError(func.token); } + else if (out_ifStatementScopeStart != nullptr) { + if(hasAllocationInIfScope(&func, scope, out_ifStatementScopeStart)) + operatorEqToSelfError(func.token); + } } } } } } +bool CheckClass::hasAllocationInIfScope(const Function *func, const Scope* scope, const Token *ifStatementScopeStart) const +{ + const Token *end; + if (ifStatementScopeStart->str() == "{") + end = ifStatementScopeStart->link(); + else + end = func->functionScope->bodyEnd; + return hasAllocation(func, scope, ifStatementScopeStart, end); +} + bool CheckClass::hasAllocation(const Function *func, const Scope* scope) const { - // This function is called when no simple check was found for assignment - // to self. We are currently looking for: - // - deallocate member ; ... member = - // - alloc member - // That is not ideal because it can cause false negatives but its currently - // necessary to prevent false positives. - const Token *last = func->functionScope->bodyEnd; - for (const Token *tok = func->functionScope->bodyStart; tok && (tok != last); tok = tok->next()) { + return hasAllocation(func, scope, func->functionScope->bodyStart, func->functionScope->bodyEnd); +} + +bool CheckClass::hasAllocation(const Function *func, const Scope* scope, const Token *start, const Token *end) const +{ + if (!end) + end = func->functionScope->bodyEnd; + for (const Token *tok = start; tok && (tok != end); tok = tok->next()) { if (Token::Match(tok, "%var% = malloc|realloc|calloc|new") && isMemberVar(scope, tok)) return true; @@ -1541,7 +1555,7 @@ bool CheckClass::hasAllocation(const Function *func, const Scope* scope) const continue; // Check for assignment to the deleted pointer (only if its a member of the class) if (isMemberVar(scope, var)) { - for (const Token *tok1 = var->next(); tok1 && (tok1 != last); tok1 = tok1->next()) { + for (const Token *tok1 = var->next(); tok1 && (tok1 != end); tok1 = tok1->next()) { if (Token::Match(tok1, "%varid% =", var->varId())) return true; } @@ -1551,7 +1565,42 @@ bool CheckClass::hasAllocation(const Function *func, const Scope* scope) const return false; } -bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs) +bool CheckClass::isInverseAssignmentTest(const Token *tok) +{ + bool res = true; + for (const Token *itr = tok; itr; itr=itr->astParent()) { + if (itr->str() == "!=" && (itr->astOperand1()->str() == "true" || itr->astOperand2()->str() == "true")) { + res = !res; + } + else if (itr->str() == "!=" && (itr->astOperand1()->str() != "false" && itr->astOperand2()->str() != "false")) { + res = !res; + } + else if (itr->str() == "!") { + res = !res; + } + else if (itr->str() == "==" && (itr->astOperand1()->str() == "false" || itr->astOperand2()->str() == "false")) { + res = !res; + } + } + return res; +} + +const Token * CheckClass::getScopeStartToken(const Token *tok) +{ + const Token *itr; + for (itr = tok; itr->astParent(); itr=itr->astParent()){} + if (itr->str() == "(" && itr->link()->next() && itr->link()->next()->str() == "{") { + if (isInverseAssignmentTest(tok)) { + return itr->link()->next(); + } + else { + return itr->link()->next()->link(); + } + } + return nullptr; +} + +bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs, const Token **out_ifStatementScopeStart) { if (!rhs) return false; @@ -1573,6 +1622,9 @@ bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs) return ChildrenToVisit::op1_and_op2; if (tok2 && tok2->isUnaryOp("&") && tok2->astOperand1()->str() == rhs->str()) ret = true; + if (ret) { + *out_ifStatementScopeStart = getScopeStartToken(tok2); + } return ret ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2; }); if (ret) diff --git a/lib/checkclass.h b/lib/checkclass.h index cedd8f4fa16..496c808d4f6 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -259,7 +259,11 @@ class CPPCHECKLIB CheckClass : public Check { // operatorEqToSelf helper functions bool hasAllocation(const Function *func, const Scope* scope) const; - static bool hasAssignSelf(const Function *func, const Token *rhs); + bool hasAllocation(const Function *func, const Scope* scope, const Token *start, const Token *last) const; + bool hasAllocationInIfScope(const Function *func, const Scope* scope, const Token *ifStatementScopeStart) const; + static bool hasAssignSelf(const Function *func, const Token *rhs, const Token **out_ifStatementScopeStart); + static bool isInverseAssignmentTest(const Token *tok); + static const Token * getScopeStartToken(const Token *tok); // checkConst helper functions bool isMemberVar(const Scope *scope, const Token *tok) const; diff --git a/test/testclass.cpp b/test/testclass.cpp index 515525c5695..9f344c76d17 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -1534,6 +1534,145 @@ class TestClass : public TestFixture { "}"); ASSERT_EQUALS("", errout.str()); + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if (&a == this)\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if ((&a == this) == true)\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if ((&a == this) != false)\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if (!((&a == this) == false))\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if ((&a != this) == false)\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if (&a != this)\n" + " {\n" + " }\n" + " else\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test and has the inverse test + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " if (&a != this)\n" + " free(s);\n" + " else\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + + // this test needs an assignment test but doesn’t have it checkOpertorEqToSelf( "class A\n" From ef20a7427938d92360cfa09cfec96cc679c44cf5 Mon Sep 17 00:00:00 2001 From: alonlib12 Date: Wed, 27 Jan 2021 19:25:30 +0200 Subject: [PATCH 2/5] Fix 'hasAllocation' function declaration --- lib/checkclass.h | 2 +- test/testclass.cpp | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/checkclass.h b/lib/checkclass.h index 496c808d4f6..8d85e9e5f0b 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -259,7 +259,7 @@ class CPPCHECKLIB CheckClass : public Check { // operatorEqToSelf helper functions bool hasAllocation(const Function *func, const Scope* scope) const; - bool hasAllocation(const Function *func, const Scope* scope, const Token *start, const Token *last) const; + bool hasAllocation(const Function *func, const Scope* scope, const Token *start, const Token *end) const; bool hasAllocationInIfScope(const Function *func, const Scope* scope, const Token *ifStatementScopeStart) const; static bool hasAssignSelf(const Function *func, const Token *rhs, const Token **out_ifStatementScopeStart); static bool isInverseAssignmentTest(const Token *tok); diff --git a/test/testclass.cpp b/test/testclass.cpp index 9f344c76d17..bfcdd95cbbd 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -1551,7 +1551,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator='123 should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test and has the inverse test checkOpertorEqToSelf( @@ -1570,7 +1570,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' 123should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test and has the inverse test checkOpertorEqToSelf( @@ -1589,7 +1589,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' 123should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test and has the inverse test checkOpertorEqToSelf( @@ -1608,7 +1608,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' 123should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test and has the inverse test checkOpertorEqToSelf( @@ -1627,7 +1627,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' 123should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test and has the inverse test checkOpertorEqToSelf( @@ -1649,7 +1649,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' 123should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test and has the inverse test checkOpertorEqToSelf( @@ -1670,7 +1670,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' 123should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test but doesn’t have it From f62afd82e17c16fcd609b50d5d73d3fdfe0a4285 Mon Sep 17 00:00:00 2001 From: alonlib12 Date: Wed, 27 Jan 2021 19:40:07 +0200 Subject: [PATCH 3/5] Fix tests asserts --- test/testclass.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/testclass.cpp b/test/testclass.cpp index bfcdd95cbbd..9f344c76d17 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -1551,7 +1551,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator='123 should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test and has the inverse test checkOpertorEqToSelf( @@ -1570,7 +1570,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' 123should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test and has the inverse test checkOpertorEqToSelf( @@ -1589,7 +1589,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' 123should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test and has the inverse test checkOpertorEqToSelf( @@ -1608,7 +1608,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' 123should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test and has the inverse test checkOpertorEqToSelf( @@ -1627,7 +1627,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' 123should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test and has the inverse test checkOpertorEqToSelf( @@ -1649,7 +1649,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' 123should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test and has the inverse test checkOpertorEqToSelf( @@ -1670,7 +1670,7 @@ class TestClass : public TestFixture { " }\n" " return *this;\n" "}"); - ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' 123should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.\n", errout.str()); // this test needs an assignment test but doesn’t have it From cd78a0a631c52bf26f2150fa04c3e1fdce244a1d Mon Sep 17 00:00:00 2001 From: alonlib12 Date: Sat, 30 Jan 2021 21:30:34 +0200 Subject: [PATCH 4/5] Fix code review rejections --- lib/checkclass.cpp | 35 ++++++++++++++++++++++++----------- lib/checkclass.h | 4 +++- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 810893ff6fd..4a8013f8318 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1565,36 +1565,49 @@ bool CheckClass::hasAllocation(const Function *func, const Scope* scope, const T return false; } +bool CheckClass::isTrueKeyword(const Token* tok) +{ + return tok->hasKnownIntValue() && tok->getKnownIntValue() == 1; +} + +bool CheckClass::isFalseKeyword(const Token* tok) +{ + return tok->hasKnownIntValue() && tok->getKnownIntValue() == 0; +} + +/* + * Checks if self-assignment test is inverse + * for example 'if (this == &rhs)' + */ bool CheckClass::isInverseAssignmentTest(const Token *tok) { bool res = true; for (const Token *itr = tok; itr; itr=itr->astParent()) { - if (itr->str() == "!=" && (itr->astOperand1()->str() == "true" || itr->astOperand2()->str() == "true")) { + if (Token::simpleMatch(itr, "!=") && (isTrueKeyword(itr->astOperand1()) || isTrueKeyword(itr->astOperand2()))) { res = !res; } - else if (itr->str() == "!=" && (itr->astOperand1()->str() != "false" && itr->astOperand2()->str() != "false")) { + else if (Token::simpleMatch(itr, "!=") && (!isFalseKeyword(itr->astOperand1()) && !isFalseKeyword(itr->astOperand2()))) { res = !res; } - else if (itr->str() == "!") { + else if (Token::simpleMatch(itr, "!")) { res = !res; } - else if (itr->str() == "==" && (itr->astOperand1()->str() == "false" || itr->astOperand2()->str() == "false")) { + else if (Token::simpleMatch(itr, "==") && (isFalseKeyword(itr->astOperand1()) || isFalseKeyword(itr->astOperand2()))) { res = !res; } } return res; } -const Token * CheckClass::getScopeStartToken(const Token *tok) +const Token * CheckClass::getIfStmtScopeStart(const Token *tok) { - const Token *itr; - for (itr = tok; itr->astParent(); itr=itr->astParent()){} - if (itr->str() == "(" && itr->link()->next() && itr->link()->next()->str() == "{") { + const Token *top = tok->astTop(); + if (Token::simpleMatch(top->link(), ") {")) { if (isInverseAssignmentTest(tok)) { - return itr->link()->next(); + return top->link()->next(); } else { - return itr->link()->next()->link(); + return top->link()->next()->link(); } } return nullptr; @@ -1623,7 +1636,7 @@ bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs, const Tok if (tok2 && tok2->isUnaryOp("&") && tok2->astOperand1()->str() == rhs->str()) ret = true; if (ret) { - *out_ifStatementScopeStart = getScopeStartToken(tok2); + *out_ifStatementScopeStart = getIfStmtScopeStart(tok2); } return ret ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2; }); diff --git a/lib/checkclass.h b/lib/checkclass.h index 8d85e9e5f0b..c59b41ca3f0 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -263,7 +263,9 @@ class CPPCHECKLIB CheckClass : public Check { bool hasAllocationInIfScope(const Function *func, const Scope* scope, const Token *ifStatementScopeStart) const; static bool hasAssignSelf(const Function *func, const Token *rhs, const Token **out_ifStatementScopeStart); static bool isInverseAssignmentTest(const Token *tok); - static const Token * getScopeStartToken(const Token *tok); + static const Token * getIfStmtScopeStart(const Token *tok); + static bool isTrueKeyword(const Token* tok); + static bool isFalseKeyword(const Token* tok); // checkConst helper functions bool isMemberVar(const Scope *scope, const Token *tok) const; From 7e42a71e3ec158dea7ee66ba06dab781801bd0a0 Mon Sep 17 00:00:00 2001 From: alonlib12 Date: Sun, 31 Jan 2021 22:44:46 +0200 Subject: [PATCH 5/5] Fix code review rejections. --- lib/checkclass.cpp | 43 +++++++++++++++++++++++++++++-------------- lib/checkclass.h | 7 +++---- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 4a8013f8318..3ccf758419a 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1565,49 +1565,64 @@ bool CheckClass::hasAllocation(const Function *func, const Scope* scope, const T return false; } -bool CheckClass::isTrueKeyword(const Token* tok) +static bool isTrueKeyword(const Token* tok) { return tok->hasKnownIntValue() && tok->getKnownIntValue() == 1; } -bool CheckClass::isFalseKeyword(const Token* tok) +static bool isFalseKeyword(const Token* tok) { return tok->hasKnownIntValue() && tok->getKnownIntValue() == 0; } /* * Checks if self-assignment test is inverse - * for example 'if (this == &rhs)' + * For example 'if (this == &rhs)' */ -bool CheckClass::isInverseAssignmentTest(const Token *tok) +CheckClass::Bool CheckClass::isInverted(const Token *tok, const Token *rhs) { bool res = true; - for (const Token *itr = tok; itr; itr=itr->astParent()) { + for (const Token *itr = tok; itr && itr->str()!="("; itr=itr->astParent()) { if (Token::simpleMatch(itr, "!=") && (isTrueKeyword(itr->astOperand1()) || isTrueKeyword(itr->astOperand2()))) { res = !res; } - else if (Token::simpleMatch(itr, "!=") && (!isFalseKeyword(itr->astOperand1()) && !isFalseKeyword(itr->astOperand2()))) { + else if (Token::simpleMatch(itr, "!=") && ( (Token::simpleMatch(itr->astOperand1(), "this") && Token::simpleMatch(itr->astOperand2(), "&") && Token::simpleMatch(itr->astOperand2()->next(), rhs->str().c_str(), rhs->str().size())) + || (Token::simpleMatch(itr->astOperand2(), "this") && Token::simpleMatch(itr->astOperand1(), "&") && Token::simpleMatch(itr->astOperand1()->next(), rhs->str().c_str(), rhs->str().size())))) { res = !res; } + else if (Token::simpleMatch(itr, "!=") && (isFalseKeyword(itr->astOperand1()) || isFalseKeyword(itr->astOperand2()))) { + //Do nothing + } else if (Token::simpleMatch(itr, "!")) { res = !res; } else if (Token::simpleMatch(itr, "==") && (isFalseKeyword(itr->astOperand1()) || isFalseKeyword(itr->astOperand2()))) { res = !res; } + else if (Token::simpleMatch(itr, "==") && (isTrueKeyword(itr->astOperand1()) || isTrueKeyword(itr->astOperand2()))) { + //Do nothing + } + else if (Token::simpleMatch(itr, "==") && ( (Token::simpleMatch(itr->astOperand1(), "this") && Token::simpleMatch(itr->astOperand2(), "&") && Token::simpleMatch(itr->astOperand2()->next(), rhs->str().c_str(), rhs->str().size())) + || (Token::simpleMatch(itr->astOperand2(), "this") && Token::simpleMatch(itr->astOperand1(), "&") && Token::simpleMatch(itr->astOperand1()->next(), rhs->str().c_str(), rhs->str().size())))) { + //Do nothing + } + else { + return Bool::BAILOUT; + } } - return res; + if (res) + return Bool::TRUE; + return Bool::FALSE; } -const Token * CheckClass::getIfStmtScopeStart(const Token *tok) +const Token * CheckClass::getIfStmtBodyStart(const Token *tok, const Token *rhs) { const Token *top = tok->astTop(); if (Token::simpleMatch(top->link(), ") {")) { - if (isInverseAssignmentTest(tok)) { - return top->link()->next(); - } - else { - return top->link()->next()->link(); + switch (isInverted(tok->astParent(), rhs)) { + case Bool::BAILOUT: return nullptr; + case Bool::TRUE: return top->link()->next(); + case Bool::FALSE: return top->link()->next()->link(); } } return nullptr; @@ -1636,7 +1651,7 @@ bool CheckClass::hasAssignSelf(const Function *func, const Token *rhs, const Tok if (tok2 && tok2->isUnaryOp("&") && tok2->astOperand1()->str() == rhs->str()) ret = true; if (ret) { - *out_ifStatementScopeStart = getIfStmtScopeStart(tok2); + *out_ifStatementScopeStart = getIfStmtBodyStart(tok2, rhs); } return ret ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2; }); diff --git a/lib/checkclass.h b/lib/checkclass.h index c59b41ca3f0..0b1bb457c07 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -262,10 +262,9 @@ class CPPCHECKLIB CheckClass : public Check { bool hasAllocation(const Function *func, const Scope* scope, const Token *start, const Token *end) const; bool hasAllocationInIfScope(const Function *func, const Scope* scope, const Token *ifStatementScopeStart) const; static bool hasAssignSelf(const Function *func, const Token *rhs, const Token **out_ifStatementScopeStart); - static bool isInverseAssignmentTest(const Token *tok); - static const Token * getIfStmtScopeStart(const Token *tok); - static bool isTrueKeyword(const Token* tok); - static bool isFalseKeyword(const Token* tok); + enum class Bool { TRUE, FALSE, BAILOUT }; + static Bool isInverted(const Token *tok, const Token *rhs); + static const Token * getIfStmtBodyStart(const Token *tok, const Token *rhs); // checkConst helper functions bool isMemberVar(const Scope *scope, const Token *tok) const;