From c9f5117cf53e7df26e613ffade931e8ae5406a41 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Tue, 13 Dec 2011 21:42:38 +0100 Subject: [PATCH] Fixed #3407 (False positive: (inconclusive) Found duplicate branches for if and else. (inline assembler)) --- lib/checkother.cpp | 53 ++++++++++-------------------------------- lib/checkuninitvar.cpp | 2 +- lib/checkunusedvar.cpp | 2 +- lib/token.cpp | 26 +++++++++++++++++++-- lib/token.h | 1 + lib/tokenize.cpp | 47 +++++++++++++++++++++---------------- test/testother.cpp | 26 +++++++++++++++++++++ test/testtokenize.cpp | 26 ++++++++++----------- 8 files changed, 105 insertions(+), 78 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 8afd2b5c6..94c668979 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2233,37 +2233,6 @@ void CheckOther::incorrectStringBooleanError(const Token *tok, const std::string // check for duplicate expressions in if statements // if (a) { } else if (a) { } //----------------------------------------------------------------------------- -static const std::string stringifyTokens(const Token *start, const Token *end) -{ - const Token *tok = start; - std::string stringified; - - if (tok->isUnsigned()) - stringified.append("unsigned "); - else if (tok->isSigned()) - stringified.append("signed "); - - if (tok->isLong()) - stringified.append("long "); - - stringified.append(tok->str()); - - while (tok && tok->next() && tok != end) { - if (tok->isUnsigned()) - stringified.append("unsigned "); - else if (tok->isSigned()) - stringified.append("signed "); - - if (tok->isLong()) - stringified.append("long "); - - tok = tok->next(); - stringified.append(" "); - stringified.append(tok->str()); - } - - return stringified; -} static bool expressionHasSideEffects(const Token *first, const Token *last) { @@ -2301,7 +2270,7 @@ void CheckOther::checkDuplicateIf() std::map expressionMap; // get the expression from the token stream - std::string expression = stringifyTokens(tok->tokAt(2), tok->next()->link()->previous()); + std::string expression = tok->tokAt(2)->stringify(tok->next()->link()); // save the expression and its location expressionMap.insert(std::make_pair(expression, tok)); @@ -2313,7 +2282,7 @@ void CheckOther::checkDuplicateIf() while (Token::simpleMatch(tok1, "} else if (") && Token::simpleMatch(tok1->linkAt(3), ") {")) { // get the expression from the token stream - expression = stringifyTokens(tok1->tokAt(4), tok1->linkAt(3)->previous()); + expression = tok1->tokAt(4)->stringify(tok1->linkAt(3)); // try to look up the expression to check for duplicates std::map::iterator it = expressionMap.find(expression); @@ -2356,28 +2325,30 @@ void CheckOther::checkDuplicateBranch() if (!_settings->isEnabled("style")) return; - if (!_settings->inconclusive) - return; - const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); std::list::const_iterator scope; for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { - const Token* const tok = scope->classDef; + const Token* tok = scope->classDef; + + if ((scope->type != Scope::eIf && scope->type != Scope::eElseIf) || !tok) + continue; + + if (tok->str() == "else") + tok = tok->next(); // check all the code in the function for if (..) else - if (scope->type == Scope::eIf && tok && tok->next() && - Token::simpleMatch(tok->next()->link(), ") {") && + if (tok && tok->next() && Token::simpleMatch(tok->next()->link(), ") {") && Token::simpleMatch(tok->next()->link()->next()->link(), "} else {")) { // save if branch code - std::string branch1 = stringifyTokens(tok->next()->link()->tokAt(2), tok->next()->link()->next()->link()->previous()); + std::string branch1 = tok->next()->link()->tokAt(2)->stringify(tok->next()->link()->next()->link()); // find else branch const Token *tok1 = tok->next()->link()->next()->link(); // save else branch code - std::string branch2 = stringifyTokens(tok1->tokAt(3), tok1->linkAt(2)->previous()); + std::string branch2 = tok1->tokAt(3)->stringify(tok1->linkAt(2)); // check for duplicates if (branch1 == branch2) diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index 3ef95d961..9fc49cd0d 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -633,7 +633,7 @@ private: return tok.next()->link(); } - if (Token::simpleMatch(&tok, "asm ( )")) { + if (Token::Match(&tok, "asm ( %str% )")) { ExecutionPath::bailOut(checks); return &tok; } diff --git a/lib/checkunusedvar.cpp b/lib/checkunusedvar.cpp index 5c358b748..cd75bd4a9 100644 --- a/lib/checkunusedvar.cpp +++ b/lib/checkunusedvar.cpp @@ -697,7 +697,7 @@ void CheckUnusedVar::checkFunctionVariableUsage() break; } - if (Token::Match(tok, "[;{}] asm ( ) ;")) { + if (Token::Match(tok, "[;{}] asm ( %str% )")) { variables.clear(); break; } diff --git a/lib/token.cpp b/lib/token.cpp index c985707af..53b3c594b 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -889,6 +889,30 @@ void Token::printOut(const char *title, const std::vector &fileName std::cout << stringifyList(true, title, fileNames) << std::endl; } +std::string Token::stringify(const Token* end) const +{ + std::ostringstream ret; + + if (isUnsigned()) + ret << "unsigned "; + else if (isSigned()) + ret << "signed "; + if (isLong()) + ret << "long "; + ret << str(); + + for (const Token *tok = this->next(); tok && tok != end; tok = tok->next()) { + if (tok->isUnsigned()) + ret << " unsigned"; + else if (tok->isSigned()) + ret << " signed"; + if (tok->isLong()) + ret << " long"; + ret << " " << tok->str(); + } + return ret.str(); +} + std::string Token::stringifyList(bool varid, const char *title) const { const std::vector fileNames; @@ -938,5 +962,3 @@ std::string Token::stringifyList(bool varid, const char *title, const std::vecto return ret.str(); } - - diff --git a/lib/token.h b/lib/token.h index c9a1c9374..c0e0d4732 100644 --- a/lib/token.h +++ b/lib/token.h @@ -326,6 +326,7 @@ public: static void replace(Token *replaceThis, Token *start, Token *end); /** Stringify a token list (with or without varId) */ + std::string stringify(const Token* end) const; std::string stringifyList(bool varid = false, const char *title = 0) const; std::string stringifyList(bool varid, const char *title, const std::vector &fileNames) const; diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp index 3e446df50..2233d897d 100644 --- a/lib/tokenize.cpp +++ b/lib/tokenize.cpp @@ -2008,18 +2008,20 @@ bool Tokenizer::tokenize(std::istream &code, for (Token *tok = _tokens; tok; tok = tok->next()) { if (Token::simpleMatch(tok, "EXEC SQL")) { // delete all tokens until ";" - const Token *end = tok; + const Token *end = tok->tokAt(2); while (end && end->str() != ";") end = end->next(); + std::string instruction = tok->stringify(end); Token::eraseTokens(tok, end); - // insert "asm ( ) ;" + // insert "asm ( "instruction" ) ;" tok->str("asm"); // it can happen that 'end' is NULL when wrong code is inserted if (!tok->next()) tok->insertToken(";"); tok->insertToken(")"); + tok->insertToken("\"" + instruction + "\""); tok->insertToken("("); // jump to ';' and continue tok = tok->tokAt(3); @@ -3054,9 +3056,9 @@ void Tokenizer::simplifyTemplatesInstantiate(const Token *tok, std::list &used, std::set &expandedtemplates) { - // this variable is not used at the moment. the intention was to + // this variable is not used at the moment. The intention was to // allow continuous instantiations until all templates has been expanded - bool done = false; + //bool done = false; std::vector type; for (tok = tok->tokAt(2); tok && tok->str() != ">"; tok = tok->next()) { @@ -3277,8 +3279,8 @@ void Tokenizer::simplifyTemplatesInstantiate(const Token *tok, // copy addtoken(tok3, tok3->linenr(), tok3->fileIndex()); if (Token::Match(tok3, "%type% <")) { - if (!Token::simpleMatch(tok3, (name + " <").c_str())) - done = false; + //if (!Token::simpleMatch(tok3, (name + " <").c_str())) + //done = false; used.push_back(_tokensBack); } @@ -3396,10 +3398,10 @@ void Tokenizer::simplifyTemplates() simplifyTemplatesUseDefaultArgumentValues(templates, used); // expand templates - bool done = false; + //bool done = false; //while (!done) { - done = true; + //done = true; for (std::list::reverse_iterator iter1 = templates.rbegin(); iter1 != templates.rend(); ++iter1) { simplifyTemplatesInstantiate(*iter1, used, expandedtemplates); } @@ -9325,39 +9327,44 @@ void Tokenizer::simplifyAssignmentInFunctionCall() // Remove __asm.. void Tokenizer::simplifyAsm() { + std::string instruction; for (Token *tok = _tokens; tok; tok = tok->next()) { - if (Token::Match(tok->next(), "__asm|_asm|asm {") && - tok->linkAt(2)->next()) { - Token::eraseTokens(tok, tok->linkAt(2)->next()); + if (Token::Match(tok, "__asm|_asm|asm {") && + tok->linkAt(1)->next()) { + instruction = tok->tokAt(2)->stringify(tok->linkAt(1)); + Token::eraseTokens(tok, tok->linkAt(1)->next()); } - else if (Token::Match(tok->next(), "asm|__asm|__asm__ volatile|__volatile__| (")) { + else if (Token::Match(tok, "asm|__asm|__asm__ volatile|__volatile__| (")) { // Goto "(" - Token *partok = tok->tokAt(2); + Token *partok = tok->next(); if (partok->str() != "(") partok = partok->next(); + instruction = partok->next()->stringify(partok->link()); Token::eraseTokens(tok, partok->link()->next()); } - else if (Token::simpleMatch(tok->next(), "__asm")) { - const Token *tok2 = tok->next(); + else if (Token::simpleMatch(tok, "__asm")) { + const Token *tok2 = tok; while (tok2 && (tok2->isNumber() || tok2->isName() || tok2->str() == ",")) tok2 = tok2->next(); - if (tok2 && tok2->str() == ";") + if (tok2 && tok2->str() == ";") { + instruction = tok->next()->stringify(tok2); Token::eraseTokens(tok, tok2); - else + } else continue; } else continue; - // insert "asm ( )" + // insert "asm ( "instruction" )" + tok->str("asm"); tok->insertToken(")"); + tok->insertToken("\"" + instruction + "\""); tok->insertToken("("); - tok->insertToken("asm"); - Token::createMutualLinks(tok->tokAt(2), tok->tokAt(3)); + Token::createMutualLinks(tok->tokAt(1), tok->tokAt(3)); //move the new tokens in the same line as ";" if available tok = tok->tokAt(3); diff --git a/test/testother.cpp b/test/testother.cpp index cfacb6094..96d6324f8 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3717,6 +3717,16 @@ private: "}"); ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:3]: (style) Found duplicate branches for if and else.\n", errout.str()); + check("void f(int a, int &b) {\n" + " if (a == 1)\n" + " b = 1;\n" + " else if (a)\n" + " b = 2;\n" + " else\n" + " b = 2;\n" + "}"); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:4]: (style) Found duplicate branches for if and else.\n", errout.str()); + check("int f(int signed, unsigned char value) {\n" " int ret;\n" " if (signed)\n" @@ -3726,6 +3736,22 @@ private: " return ret;\n" "}"); ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " if (b)\n" + " __asm__(\"mov ax, bx\");\n" + " else\n" + " __asm__(\"mov bx, bx\");\n" + "}"); + ASSERT_EQUALS("", errout.str()); // #3407 + + check("void f() {\n" + " if (b)\n" + " __asm__(\"mov ax, bx\");\n" + " else\n" + " __asm__(\"mov ax, bx\");\n" + "}"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:2]: (style) Found duplicate branches for if and else.\n", errout.str()); } void duplicateExpression1() { diff --git a/test/testtokenize.cpp b/test/testtokenize.cpp index 6be3ceb95..102b77e28 100644 --- a/test/testtokenize.cpp +++ b/test/testtokenize.cpp @@ -741,19 +741,19 @@ private: } void inlineasm() { - ASSERT_EQUALS("; asm ( ) ;", tokenizeAndStringify(";asm { mov ax,bx };")); - ASSERT_EQUALS("; asm ( ) ;", tokenizeAndStringify(";_asm { mov ax,bx };")); - ASSERT_EQUALS("; asm ( ) ;", tokenizeAndStringify(";__asm { mov ax,bx };")); - ASSERT_EQUALS("; asm ( ) ;", tokenizeAndStringify(";__asm__ __volatile__ ( \"mov ax,bx\" );")); - ASSERT_EQUALS("; asm ( ) ;", tokenizeAndStringify(";__asm _emit 12h ;")); - ASSERT_EQUALS("; asm ( ) ;", tokenizeAndStringify(";__asm mov a, b ;")); - ASSERT_EQUALS("; asm ( ) ;", tokenizeAndStringify(";asm volatile (\"fnstcw %0\" : \"= m\" (old_cw));")); - ASSERT_EQUALS("; asm ( ) ;", tokenizeAndStringify("; __asm__ (\"fnstcw %0\" : \"= m\" (old_cw));")); - ASSERT_EQUALS("; asm ( ) ;", tokenizeAndStringify("; __asm __volatile__ (\"ddd\") ;")); - ASSERT_EQUALS("; asm ( ) ;", tokenizeAndStringify(";__asm__ volatile ( \"mov ax,bx\" );")); + ASSERT_EQUALS("; asm ( \"mov ax , bx\" ) ;", tokenizeAndStringify(";asm { mov ax,bx };")); + ASSERT_EQUALS("; asm ( \"mov ax , bx\" ) ;", tokenizeAndStringify(";_asm { mov ax,bx };")); + ASSERT_EQUALS("; asm ( \"mov ax , bx\" ) ;", tokenizeAndStringify(";__asm { mov ax,bx };")); + ASSERT_EQUALS("; asm ( \"\"mov ax,bx\"\" ) ;", tokenizeAndStringify(";__asm__ __volatile__ ( \"mov ax,bx\" );")); + ASSERT_EQUALS("; asm ( \"_emit 12h\" ) ;", tokenizeAndStringify(";__asm _emit 12h ;")); + ASSERT_EQUALS("; asm ( \"mov a , b\" ) ;", tokenizeAndStringify(";__asm mov a, b ;")); + ASSERT_EQUALS("; asm ( \"\"fnstcw %0\" : \"= m\" ( old_cw )\" ) ;", tokenizeAndStringify(";asm volatile (\"fnstcw %0\" : \"= m\" (old_cw));")); + ASSERT_EQUALS("; asm ( \"\"fnstcw %0\" : \"= m\" ( old_cw )\" ) ;", tokenizeAndStringify("; __asm__ (\"fnstcw %0\" : \"= m\" (old_cw));")); + ASSERT_EQUALS("; asm ( \"\"ddd\"\" ) ;", tokenizeAndStringify("; __asm __volatile__ (\"ddd\") ;")); + ASSERT_EQUALS("; asm ( \"\"mov ax,bx\"\" ) ;", tokenizeAndStringify(";__asm__ volatile ( \"mov ax,bx\" );")); // 'asm ( ) ;' should be in the same line - ASSERT_EQUALS(";\n\nasm ( ) ;", tokenizeAndStringify(";\n\n__asm__ volatile ( \"mov ax,bx\" );", true)); + ASSERT_EQUALS(";\n\nasm ( \"\"mov ax,bx\"\" ) ;", tokenizeAndStringify(";\n\n__asm__ volatile ( \"mov ax,bx\" );", true)); } @@ -5797,8 +5797,8 @@ private: void sql() { // Oracle PRO*C extensions for inline SQL. Just replace the SQL with "asm()" to fix wrong error messages // ticket: #1959 - ASSERT_EQUALS("asm ( ) ;", tokenizeAndStringify("EXEC SQL SELECT A FROM B;",false)); - ASSERT_EQUALS("asm ( ) ;", tokenizeAndStringify("EXEC SQL",false)); + ASSERT_EQUALS("asm ( \"\"EXEC SQL SELECT A FROM B\"\" ) ;", tokenizeAndStringify("EXEC SQL SELECT A FROM B;",false)); + ASSERT_EQUALS("asm ( \"\"EXEC SQL\"\" ) ;", tokenizeAndStringify("EXEC SQL",false)); }