walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Merge pull request #61 from richq/sameexpr 

Fix for #3317 - same expression on both side of '&&' false positives
This commit is contained in:
Daniel Marjamäki 2011-11-29 08:50:02 -08:00
parent 932680994d d28cf42d4c
commit b538f49a6e
3 changed files with 221 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

172
lib/checkother.cpp
View File

@ -2382,59 +2382,176 @@ void CheckOther::duplicateBranchError(const Token *tok1, const Token *tok2)
}
namespace {
struct ExpressionTokens {
const Token *start;
const Token *end;
int count;
ExpressionTokens(const Token *s, const Token *e): start(s), end(e), count(1) {}
};
class Expressions {
public:
void endExpr() {
Expressions(): _start(0) {}
void endExpr(const Token *end) {
const std::string &e = _expression.str();
if (!e.empty()) {
std::map<std::string,int>::const_iterator it = _expressions.find(e);
std::map<std::string, ExpressionTokens>::iterator it = _expressions.find(e);
if (it == _expressions.end())
_expressions[e] = 1;
_expressions.insert(std::make_pair(e, ExpressionTokens(_start, end)));
else
_expressions[e] += 1;
it->second.count += 1;
}
_expression.str("");
_start = 0;
}
void append(const std::string &tok) {
_expression << tok;
void append(const Token *tok) {
if (!_start)
_start = tok;
_expression << tok->str();
}
std::map<std::string,int> &getMap() {
std::map<std::string,ExpressionTokens> &getMap() {
return _expressions;
}
private:
std::map<std::string, int> _expressions;
std::map<std::string, ExpressionTokens> _expressions;
std::ostringstream _expression;
const Token *_start;
};
struct FuncFilter {
FuncFilter(const Scope *scope, const Token *tok): _scope(scope), _tok(tok) {}
bool operator() (const Function &func) {
// todo: function args, etc??
bool matchingFunc = func.type == Function::eFunction &&
_tok->str() == func.token->str();
// either a class function, or a global function with the same name
return (_scope && _scope == func.functionScope && matchingFunc) ||
(!_scope && matchingFunc);
}
const Scope *_scope;
const Token *_tok;
};
bool inconclusiveFunctionCall(const SymbolDatabase *symbolDatabase,
const std::list<Function> &constFunctions,
const ExpressionTokens &tokens)
{
const Token *start = tokens.start;
const Token *end = tokens.end;
// look for function calls between start and end...
for (const Token *tok = start; tok && tok != end; tok = tok->next()) {
if (tok != start && tok->str() == "(") {
// go back to find the function call.
const Token *prev = tok->previous();
if (prev->str() == ">") {
// ignore template functions like boo<double>()
return true;
}
if (prev && prev->isName()) {
const Variable *v = 0;
if (Token::Match(prev->tokAt(-2), "%var% .")) {
const Token *scope = prev->tokAt(-2);
v = symbolDatabase->getVariableFromVarId(scope->varId());
}
// hard coded list of safe, no-side-effect functions
if (v == 0 && Token::Match(prev, "strcmp|strncmp|strlen|memcmp|strcasecmp|strncasecmp"))
return false;
std::list<Function>::const_iterator it = std::find_if(constFunctions.begin(),
constFunctions.end(),
FuncFilter(v ? v->type(): 0, prev));
if (it == constFunctions.end())
return true;
}
}
}
return false;
}
bool notconst(const Function &func)
{
return !func.isConst;
}
void getConstFunctions(const SymbolDatabase *symbolDatabase, std::list<Function> &constFunctions)
{
std::list<Scope>::const_iterator scope;
for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) {
std::list<Function>::const_iterator func;
// only add const functions that do not have a non-const overloaded version
// since it is pretty much impossible to tell which is being called.
typedef std::map<std::string, std::list<Function> > StringFunctionMap;
StringFunctionMap functionsByName;
for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) {
StringFunctionMap::iterator it = functionsByName.find(func->token->str());
Scope *currScope = const_cast<Scope*>(&*scope);
if (it == functionsByName.end()) {
std::list<Function> tmp;
tmp.push_back(*func);
tmp.back().functionScope = currScope;
functionsByName[func->token->str()] = tmp;
} else {
it->second.push_back(*func);
it->second.back().functionScope = currScope;
}
}
for (StringFunctionMap::iterator it = functionsByName.begin();
it != functionsByName.end(); ++it) {
std::list<Function>::const_iterator nc = std::find_if(it->second.begin(), it->second.end(), notconst);
if (nc == it->second.end()) {
// ok to add all of them
constFunctions.splice(constFunctions.end(), it->second);
}
}
}
}
}
void CheckOther::checkExpressionRange(const Token *start, const Token *end, const std::string &toCheck)
void CheckOther::checkExpressionRange(const std::list<Function> &constFunctions,
const Token *start,
const Token *end,
const std::string &toCheck)
{
if (!start || !end)
return;
Expressions expressions;
std::string opName;
int level = 0;
for (const Token *tok = start->next(); tok && tok != end; tok = tok->next()) {
if (Token::Match(tok, toCheck.c_str())) {
if (tok->str() == ")")
level--;
else if (tok->str() == "(")
level++;
if (level == 0 && Token::Match(tok, toCheck.c_str())) {
opName = tok->str();
expressions.endExpr();
expressions.endExpr(tok);
} else {
expressions.append(tok->str());
expressions.append(tok);
}
}
expressions.endExpr();
std::map<std::string,int>::const_iterator it = expressions.getMap().begin();
expressions.endExpr(end);
std::map<std::string,ExpressionTokens>::const_iterator it = expressions.getMap().begin();
const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
for (; it != expressions.getMap().end(); ++it) {
if (it->second > 1)
duplicateExpressionError(start, start, opName);
if (it->second.count > 1 &&
(it->first.find("(") == std::string::npos ||
!inconclusiveFunctionCall(symbolDatabase, constFunctions, it->second))) {
duplicateExpressionError(it->second.start, it->second.start, opName);
}
}
}
void CheckOther::complexDuplicateExpressionCheck(const Token *classStart,
const std::string &toCheck,
const std::string &alt)
void CheckOther::complexDuplicateExpressionCheck(const std::list<Function> &constFunctions,
const Token *classStart,
const std::string &toCheck,
const std::string &alt)
{
std::string statementStart(",|=|return");
if (!alt.empty())
@ -2456,7 +2573,7 @@ void CheckOther::complexDuplicateExpressionCheck(const Token *classStart,
else if (tok1->str() == "(")
level--;
if (level < 0 || Token::Match(tok1, statementStart.c_str())) {
if (level < 0 || (level == 0 && Token::Match(tok1, statementStart.c_str()))) {
start = tok1;
break;
}
@ -2470,16 +2587,15 @@ void CheckOther::complexDuplicateExpressionCheck(const Token *classStart,
else if (tok1->str() == "(")
level++;
if (level < 0 || Token::Match(tok1, statementEnd.c_str())) {
if (level < 0 || (level == 0 && Token::Match(tok1, statementEnd.c_str()))) {
end = tok1;
break;
}
}
checkExpressionRange(start, end, toCheck);
checkExpressionRange(constFunctions, start, end, toCheck);
}
}
//---------------------------------------------------------------------------
// check for the same expression on both sides of an operator
// (x == x), (x && x), (x || x)
@ -2494,16 +2610,18 @@ void CheckOther::checkDuplicateExpression()
const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
std::list<Scope>::const_iterator scope;
std::list<Function> constFunctions;
getConstFunctions(symbolDatabase, constFunctions);
for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) {
// only check functions
if (scope->type != Scope::eFunction)
continue;
complexDuplicateExpressionCheck(scope->classStart, "%or%", "");
complexDuplicateExpressionCheck(scope->classStart, "%oror%", "");
complexDuplicateExpressionCheck(scope->classStart, "&", "%oror%|%or%");
complexDuplicateExpressionCheck(scope->classStart, "&&", "%oror%|%or%");
complexDuplicateExpressionCheck(constFunctions, scope->classStart, "%or%", "");
complexDuplicateExpressionCheck(constFunctions, scope->classStart, "%oror%", "");
complexDuplicateExpressionCheck(constFunctions, scope->classStart, "&", "%oror%|%or%");
complexDuplicateExpressionCheck(constFunctions, scope->classStart, "&&", "%oror%|%or%");
for (const Token *tok = scope->classStart; tok && tok != scope->classStart->link(); tok = tok->next()) {
if (Token::Match(tok, ",|=|return|(|&&|%oror% %var% ==|!=|<=|>=|<|>|- %var% )|&&|%oror%|;|,") &&

10
lib/checkother.h
View File

@ -26,6 +26,7 @@
#include "settings.h"
class Token;
class Function;
/// @addtogroup Checks
/// @{
@ -434,8 +435,13 @@ private:
return varname;
}
void checkExpressionRange(const Token *start, const Token *end, const std::string &toCheck);
void complexDuplicateExpressionCheck(const Token *classStart,
void checkExpressionRange(const std::list<Function> &constFunctions,
const Token *start,
const Token *end,
const std::string &toCheck);
void complexDuplicateExpressionCheck(const std::list<Function> &constFunctions,
const Token *classStart,
const std::string &toCheck,
const std::string &alt);
};

73
test/testother.cpp
View File

@ -142,6 +142,7 @@ private:
TEST_CASE(duplicateBranch);
TEST_CASE(duplicateExpression1);
TEST_CASE(duplicateExpression2); // ticket #2730
TEST_CASE(duplicateExpression3); // ticket #3317
TEST_CASE(alwaysTrueFalseStringCompare);
TEST_CASE(checkStrncmpSizeof);
@ -3637,11 +3638,6 @@ private:
"}");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str());
check("void foo() {\n"
" if (this->bar() || this->bar()) {}\n"
"}");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str());
check("void foo() {\n"
" if (a && b || a && b) {}\n"
"}");
@ -3661,10 +3657,19 @@ private:
" if ((a + b) | (a + b)) {}\n"
"}");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '|'.\n", errout.str());
check("void foo() {\n"
" if ((a | b) & (a | b)) {}\n"
"}");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '&'.\n", errout.str());
check("void d(const char f, int o, int v)\n"
"{\n"
" if (((f=='R') && (o == 1) && ((v < 2) || (v > 99))) ||\n"
" ((f=='R') && (o == 2) && ((v < 2) || (v > 99))) ||\n"
" ((f=='T') && (o == 2) && ((v < 200) || (v > 9999)))) {}\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void duplicateExpression2() { // ticket #2730
@ -3687,6 +3692,64 @@ private:
"[test.cpp:9]: (error) Passing value -1.0 to sqrtf() leads to undefined result\n", errout.str());
}
void duplicateExpression3() {
check("void foo() {\n"
" if (x() || x()) {}\n"
"}");
ASSERT_EQUALS("", errout.str());
check("struct A {\n"
" void foo() const;\n"
" bool bar() const;\n"
"};\n"
"void A::foo() const {\n"
" if (bar() && bar()) {}\n"
"}");
ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:6]: (style) Same expression on both sides of '&&'.\n", errout.str());
check("struct A {\n"
" void foo();\n"
" bool bar();\n"
" bool bar() const;\n"
"};\n"
"void A::foo() {\n"
" if (bar() && bar()) {}\n"
"}");
ASSERT_EQUALS("", errout.str());
check("class B {\n"
" void bar(int i);\n"
"};\n"
"class A {\n"
" void bar(int i) const;\n"
"};\n"
"void foo() {\n"
" B b;\n"
" A a;\n"
" if (b.bar(1) && b.bar(1)) {}\n"
" if (a.bar(1) && a.bar(1)) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:11] -> [test.cpp:11]: (style) Same expression on both sides of '&&'.\n", errout.str());
check("class D { void strcmp(); };\n"
"void foo() {\n"
" D d;\n"
" if (d.strcmp() && d.strcmp()) {}\n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("void foo() {\n"
" if ((strcmp(a, b) == 0) || (strcmp(a, b) == 0)) {}\n"
"}");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str());
check("void foo() {\n"
" if (str == \"(\" || str == \"(\") {}\n"
"}");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str());
}
void alwaysTrueFalseStringCompare() {
check_preprocess_suppress(
"#define MACRO \"00FF00\"\n"