From 0ddeac0429c74e0e91029ec7ab9ee1eb3b0647f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Mon, 18 Jul 2016 12:43:23 +0200 Subject: [PATCH] refactor (use ast) and improve CheckOther::checkRedundantAssignment (warn about global variables unless they are volatile, handle arrays in lhs better) --- lib/checkother.cpp | 72 +++++++++++++++++++++++++++++++++++------- lib/symboldatabase.cpp | 2 ++ lib/symboldatabase.h | 11 ++++++- test/testother.cpp | 21 +++++++----- 4 files changed, 85 insertions(+), 21 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 5abba0d68..c56b6ae96 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -407,6 +407,13 @@ static bool nonLocal(const Variable* var) return !var || (!var->isLocal() && !var->isArgument()) || var->isStatic() || var->isReference(); } +static bool nonLocalVolatile(const Variable* var) +{ + if (var && var->isVolatile()) + return false; + return nonLocal(var); +} + static void eraseNotLocalArg(std::map& container, const SymbolDatabase* symbolDatabase) { for (std::map::iterator i = container.begin(); i != container.end();) { @@ -493,6 +500,18 @@ void CheckOther::checkRedundantAssignment() varAssignments.clear(); memAssignments.clear(); } else if (tok->tokType() == Token::eVariable && !Token::Match(tok, "%name% (")) { + const Token *eq = nullptr; + for (const Token *tok2 = tok; tok2; tok2 = tok2->next()) { + if (Token::Match(tok2, "[([]")) + tok2 = tok2->link(); + else if (Token::Match(tok2, "[)];,]")) + break; + else if (tok2->str() == "=") { + eq = tok2; + break; + } + } + // Set initialization flag if (!Token::Match(tok, "%var% [")) initialized.insert(tok->varId()); @@ -512,19 +531,48 @@ void CheckOther::checkRedundantAssignment() } std::map::iterator it = varAssignments.find(tok->varId()); - if (Token::simpleMatch(tok->next(), "=") && Token::Match(startToken, "[;{}]")) { // Assignment + if (eq && Token::Match(startToken, "[;{}]")) { // Assignment if (it != varAssignments.end()) { - bool error = true; // Ensure that variable is not used on right side - for (const Token* tok2 = tok->tokAt(2); tok2; tok2 = tok2->next()) { - if (tok2->str() == ";") + const Token *oldeq = nullptr; + for (const Token *tok2 = it->second; tok2; tok2 = tok2->next()) { + if (Token::Match(tok2, "[([]")) + tok2 = tok2->link(); + else if (Token::Match(tok2, "[)];,]")) break; - else if (tok2->varId() == tok->varId()) { + else if (Token::Match(tok2, "++|--|=")) { + oldeq = tok2; + break; + } + } + if (!oldeq) { + const Token *tok2 = it->second; + while (Token::Match(tok2, "%name%|.|[|*|(")) + tok2 = tok2->astParent(); + if (Token::Match(tok2, "++|--")) + oldeq = tok2; + } + + // Ensure that LHS in assignments are the same + bool error = oldeq && isSameExpression(_tokenizer->isCPP(), true, eq->astOperand1(), oldeq->astOperand1(), _settings->library.functionpure); + + // Ensure that variable is not used on right side + std::stack tokens; + tokens.push(eq->astOperand2()); + while (!tokens.empty()) { + const Token *rhs = tokens.top(); + tokens.pop(); + if (!rhs) + continue; + tokens.push(rhs->astOperand1()); + tokens.push(rhs->astOperand2()); + if (rhs->varId() == tok->varId()) { error = false; break; - } else if (Token::Match(tok2, "%name% (") && nonLocal(tok->variable())) { // Called function might use the variable - const Function* const func = tok2->function(); + } + if (Token::Match(rhs->previous(), "%name% (") && nonLocalVolatile(tok->variable())) { // Called function might use the variable + const Function* const func = rhs->function(); const Variable* const var = tok->variable(); - if (!var || var->isGlobal() || var->isReference() || ((!func || func->nestedIn) && tok2->strAt(-1) != ".")) {// Global variable, or member function + if (!var || var->isGlobal() || var->isReference() || ((!func || func->nestedIn) && rhs->strAt(-1) != ".")) {// Global variable, or member function error = false; break; } @@ -532,13 +580,13 @@ void CheckOther::checkRedundantAssignment() } if (error) { if (printWarning && scope->type == Scope::eSwitch && Token::findmatch(it->second, "default|case", tok)) - redundantAssignmentInSwitchError(it->second, tok, tok->str()); + redundantAssignmentInSwitchError(it->second, tok, eq->astOperand1()->expressionString()); else if (printPerformance) { // See #7133 - const bool nonlocal = it->second->variable() && nonLocal(it->second->variable()); - if (printInconclusive || !nonlocal) // see #5089 - report inconclusive only when requested + const bool nonlocal = it->second->variable() && nonLocalVolatile(it->second->variable()); + if (printInconclusive || !nonlocal) // report inconclusive only when requested if (_tokenizer->isC() || checkExceptionHandling(tok)) // see #6555 to see how exception handling might have an impact - redundantAssignmentError(it->second, tok, tok->str(), nonlocal); // Inconclusive for non-local variables + redundantAssignmentError(it->second, tok, eq->astOperand1()->expressionString(), nonlocal); // Inconclusive for non-local variables } } it->second = tok; diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index a37bc7486..21add8f86 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -1613,6 +1613,8 @@ void Variable::evaluate(const Library* lib) setFlag(fIsStatic, true); else if (tok->str() == "extern") setFlag(fIsExtern, true); + else if (tok->str() == "volatile") + setFlag(fIsVolatile, true); else if (tok->str() == "mutable") setFlag(fIsMutable, true); else if (tok->str() == "const") diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 61b7bc76d..5713c5d8a 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -159,7 +159,8 @@ class CPPCHECKLIB Variable { fHasDefault = (1 << 9), /** @brief function argument with default value */ fIsStlType = (1 << 10), /** @brief STL type ('std::') */ fIsStlString = (1 << 11), /** @brief std::string|wstring|basic_string<T>|u16string|u32string */ - fIsFloatType = (1 << 12) /** @brief Floating point type */ + fIsFloatType = (1 << 12), /** @brief Floating point type */ + fIsVolatile = (1 << 13) /** @brief volatile */ }; /** @@ -339,6 +340,14 @@ public: return getFlag(fIsMutable); } + /** + * Is variable volatile. + * @return true if volatile, false if not + */ + bool isVolatile() const { + return getFlag(fIsVolatile); + } + /** * Is variable static. * @return true if static, false if not diff --git a/test/testother.cpp b/test/testother.cpp index 20e680e12..8caa36990 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -4907,6 +4907,13 @@ private: "}"); ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style, inconclusive) Variable 'i' is reassigned a value before the old one has been used if variable is no semaphore variable.\n", errout.str()); + check("void f() {\n" + " int i[10];\n" + " i[2] = 1;\n" + " i[2] = 1;\n" + "}", nullptr, false, false, false); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) Variable 'i[2]' is reassigned a value before the old one has been used.\n", errout.str()); + // Testing different types check("void f() {\n" " Foo& bar = foo();\n" @@ -5041,7 +5048,7 @@ private: " x = 2;\n" " x = z.g();\n" "}"); - ASSERT_EQUALS("[test.cpp:8] -> [test.cpp:9]: (style, inconclusive) Variable 'x' is reassigned a value before the old one has been used if variable is no semaphore variable.\n", errout.str()); + ASSERT_EQUALS("", errout.str()); // from #3103 (avoid a false negative) check("int foo(){\n" @@ -5097,7 +5104,7 @@ private: " ab.a = 2;\n" " return ab.a;\n" "}"); - ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:6]: (style, inconclusive) Variable 'a' is reassigned a value before the old one has been used if variable is no semaphore variable.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:6]: (style) Variable 'ab.a' is reassigned a value before the old one has been used.\n", errout.str()); check("struct AB { int a; int b; };\n" "\n" @@ -5257,9 +5264,7 @@ private: " catch (const uno::Exception&) {\n" " }\n" "}", "test.cpp", false, true); - TODO_ASSERT_EQUALS("", - "[test.cpp:6] -> [test.cpp:9]: (style) Variable 'Name' is reassigned a value before the old one has been used.\n", - errout.str()); + ASSERT_EQUALS("", errout.str()); check("void ConvertBitmapData(sal_uInt16 nDestBits) {\n" "BitmapBuffer aSrcBuf;\n" @@ -5268,7 +5273,7 @@ private: " aSrcBuf.mnBitCount = nDestBits;\n" " bConverted = ::ImplFastBitmapConversion( aDstBuf, aSrcBuf, aTwoRects );\n" "}", "test.c"); - ASSERT_EQUALS("[test.c:3] -> [test.c:5]: (style) Variable 'mnBitCount' is reassigned a value before the old one has been used.\n", errout.str()); + ASSERT_EQUALS("[test.c:3] -> [test.c:5]: (style) Variable 'aSrcBuf.mnBitCount' is reassigned a value before the old one has been used.\n", errout.str()); check("void ConvertBitmapData(sal_uInt16 nDestBits) {\n" "BitmapBuffer aSrcBuf;\n" " aSrcBuf.mnBitCount = nSrcBits;\n" @@ -5276,8 +5281,8 @@ private: " aSrcBuf.mnBitCount = nDestBits;\n" " bConverted = ::ImplFastBitmapConversion( aDstBuf, aSrcBuf, aTwoRects );\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:5]: (style, inconclusive) Variable 'mnBitCount' is reassigned a value before the old one has been used.\n", - "[test.cpp:3] -> [test.cpp:5]: (style) Variable 'mnBitCount' is reassigned a value before the old one has been used.\n", + TODO_ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:5]: (style, inconclusive) Variable 'aSrcBuf.mnBitCount' is reassigned a value before the old one has been used.\n", + "[test.cpp:3] -> [test.cpp:5]: (style) Variable 'aSrcBuf.mnBitCount' is reassigned a value before the old one has been used.\n", errout.str()); }