From 1e550f9fdf0f24a4ff9657fa4fd8e2f8bb3494f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 12 Feb 2013 16:13:08 +0100 Subject: [PATCH] Reverted fix for #4547: It causes fp. See #4573 --- lib/checknullpointer.cpp | 78 ++++++++++++++++++++-------------------- lib/checkother.cpp | 16 ++++----- lib/settings.cpp | 4 ++- lib/tokenize.cpp | 10 ++++-- test/testother.cpp | 6 ---- test/testtokenize.cpp | 15 -------- 6 files changed, 57 insertions(+), 72 deletions(-) diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index f1c9fe5c5..70492ffdc 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -668,57 +668,55 @@ void CheckNullPointer::nullPointerStructByDeRefAndChec() // count { and } using tok2 const Token* const end2 = tok1->scope()->classEnd; for (const Token *tok2 = tok1->tokAt(3); tok2 != end2; tok2 = tok2->next()) { + bool unknown = false; // label / ?: if (tok2->str() == ":") break; // function call.. - else { - bool unknown = false; - if (Token::Match(tok2, "[;{}] %var% (") && CanFunctionAssignPointer(tok2->next(), varid1, unknown)) { - if (!_settings->inconclusive || !unknown) - break; - inconclusive = true; - } - - // Reassignment of the struct - else if (tok2->varId() == varid1) { - if (tok2->next()->str() == "=") { - // Avoid false positives when there is 'else if' - // TODO: can this be handled better? - if (tok1->strAt(-2) == "if") - skipvar.insert(varid1); - break; - } - if (Token::Match(tok2->tokAt(-2), "[,(] &")) - break; - } - - // Loop.. - /** @todo don't bail out if the variable is not used in the loop */ - else if (tok2->str() == "do") + else if (Token::Match(tok2, "[;{}] %var% (") && CanFunctionAssignPointer(tok2->next(), varid1, unknown)) { + if (!_settings->inconclusive || !unknown) break; + inconclusive = true; + } - // return/break at base level => stop checking - else if (tok2->scope()->classEnd == end2 && (tok2->str() == "return" || tok2->str() == "break")) - break; - - // Function call: If the pointer is not a local variable it - // might be changed by the call. - else if (Token::Match(tok2, "[;{}] %var% (") && - Token::simpleMatch(tok2->linkAt(2), ") ;") && !isLocal) { + // Reassignment of the struct + else if (tok2->varId() == varid1) { + if (tok2->next()->str() == "=") { + // Avoid false positives when there is 'else if' + // TODO: can this be handled better? + if (tok1->strAt(-2) == "if") + skipvar.insert(varid1); break; } - - // Check if pointer is null. - // TODO: false negatives for "if (!p || .." - else if (!tok2->isExpandedMacro() && Token::Match(tok2, "if ( !| %varid% )|&&", varid1)) { - // Is this variable a pointer? - if (var->isPointer()) - nullPointerError(tok1, varname, tok2, inconclusive); + if (Token::Match(tok2->tokAt(-2), "[,(] &")) break; - } + } + + // Loop.. + /** @todo don't bail out if the variable is not used in the loop */ + else if (tok2->str() == "do") + break; + + // return/break at base level => stop checking + else if (tok2->scope()->classEnd == end2 && (tok2->str() == "return" || tok2->str() == "break")) + break; + + // Function call: If the pointer is not a local variable it + // might be changed by the call. + else if (Token::Match(tok2, "[;{}] %var% (") && + Token::simpleMatch(tok2->linkAt(2), ") ;") && !isLocal) { + break; + } + + // Check if pointer is null. + // TODO: false negatives for "if (!p || .." + else if (!tok2->isExpandedMacro() && Token::Match(tok2, "if ( !| %varid% )|&&", varid1)) { + // Is this variable a pointer? + if (var->isPointer()) + nullPointerError(tok1, varname, tok2, inconclusive); + break; } } } diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 487176a08..cb19fcfe4 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2616,14 +2616,14 @@ void CheckOther::checkDuplicateIf() // save the expression and its location expressionMap.insert(std::make_pair(expression, tok)); - // find the next else { if (...) statement + // find the next else if (...) statement const Token *tok1 = scope->classEnd; - // check all the else { if (...) statements - while (Token::simpleMatch(tok1, "} else { if (") && - Token::simpleMatch(tok1->linkAt(4), ") {")) { + // check all the else if (...) statements + while (Token::simpleMatch(tok1, "} else if (") && + Token::simpleMatch(tok1->linkAt(3), ") {")) { // get the expression from the token stream - expression = tok1->tokAt(5)->stringifyList(tok1->linkAt(4)); + expression = tok1->tokAt(4)->stringifyList(tok1->linkAt(3)); // try to look up the expression to check for duplicates std::map::iterator it = expressionMap.find(expression); @@ -2631,7 +2631,7 @@ void CheckOther::checkDuplicateIf() // found a duplicate if (it != expressionMap.end()) { // check for expressions that have side effects and ignore them - if (!expressionHasSideEffects(tok1->tokAt(5), tok1->linkAt(4)->previous())) + if (!expressionHasSideEffects(tok1->tokAt(4), tok1->linkAt(3)->previous())) duplicateIfError(it->second, tok1->next()); } @@ -2639,8 +2639,8 @@ void CheckOther::checkDuplicateIf() else expressionMap.insert(std::make_pair(expression, tok1->next())); - // find the next else { if (...) statement - tok1 = tok1->linkAt(4)->next()->link(); + // find the next else if (...) statement + tok1 = tok1->linkAt(3)->next()->link(); } } } diff --git a/lib/settings.cpp b/lib/settings.cpp index 6cae4201d..db0b45240 100644 --- a/lib/settings.cpp +++ b/lib/settings.cpp @@ -71,6 +71,8 @@ std::string Settings::addEnabled(const std::string &str) return addEnabled(str.substr(prevPos)); } + bool handled = false; + static std::set id; if (id.empty()) { id.insert("warning"); @@ -98,7 +100,7 @@ std::string Settings::addEnabled(const std::string &str) if (str == "information") { _enabled.insert("missingInclude"); } - } else { + } else if (!handled) { if (str.empty()) return std::string("cppcheck: --enable parameter is empty"); else diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp index fc80f4709..cc04088cc 100644 --- a/lib/tokenize.cpp +++ b/lib/tokenize.cpp @@ -4101,8 +4101,14 @@ Token *Tokenizer::simplifyAddBracesToCommand(Token *tok) if (!tokEnd) return NULL; Token * tokEndNext=tokEnd->next(); - if (tokEndNext && tokEndNext->str()=="else") - tokEnd=simplifyAddBracesPair(tokEndNext,false); + if (tokEndNext && tokEndNext->str()=="else") { + Token * tokEndNextNext=tokEndNext->next(); + if (tokEndNextNext && tokEndNextNext->str()=="if") { + // do not change "else if ..." to "else { if ... }" + tokEnd=simplifyAddBracesToCommand(tokEndNextNext); + } else + tokEnd=simplifyAddBracesPair(tokEndNext,false); + } } return tokEnd; diff --git a/test/testother.cpp b/test/testother.cpp index f83894a49..469a1672b 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -4937,12 +4937,6 @@ private: "}"); ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (style) Duplicate conditions in 'if' and related 'else if'.\n", errout.str()); - check("void f(int a, int &b) {\n" - " if (a) { b = 1; }\n" - " else { if (a) { b = 2; } }\n" - "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (style) Duplicate conditions in 'if' and related 'else if'.\n", errout.str()); - check("void f(int a, int &b) {\n" " if (a == 1) { b = 1; }\n" " else if (a == 2) { b = 2; }\n" diff --git a/test/testtokenize.cpp b/test/testtokenize.cpp index b00b08b76..459f22563 100644 --- a/test/testtokenize.cpp +++ b/test/testtokenize.cpp @@ -108,7 +108,6 @@ private: TEST_CASE(ifAddBraces17); // '} else' should be in the same line TEST_CASE(ifAddBraces18); // #3424 - if if { } else else TEST_CASE(ifAddBraces19); // #3928 - if for if else - TEST_CASE(ifAddBraces20); TEST_CASE(whileAddBraces); TEST_CASE(doWhileAddBraces); @@ -1219,20 +1218,6 @@ private: "}", tokenizeAndStringify(code, true)); } - void ifAddBraces20() { - // if else if - const char code[] = "void f()\n" - "{\n" - " if (a);\n" - " else if (b);\n" - "}\n"; - ASSERT_EQUALS("void f ( )\n" - "{\n" - "if ( a ) { ; }\n" - "else { if ( b ) { ; } }\n" - "}", tokenizeAndStringify(code, true)); - } - void whileAddBraces() { const char code[] = ";while(a);";