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

Fix #11862 FN constParameterPointer with increment (#5291)

This commit is contained in:
chrchr-github 2023-08-08 10:38:03 +02:00 committed by GitHub
parent 610777d586
commit 838b6b86e3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 58 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
lib/astutils.cpp
View File

@ -2188,7 +2188,7 @@ T* getTokenArgumentFunctionImpl(T* tok, int& argn)
argn = -1; argn = -1;
{ {
T* parent = tok->astParent(); T* parent = tok->astParent();
if (parent && parent->isUnaryOp("&")) if (parent && (parent->isUnaryOp("&") || parent->isIncDecOp()))
parent = parent->astParent(); parent = parent->astParent();
while (parent && parent->isCast()) while (parent && parent->isCast())
parent = parent->astParent(); parent = parent->astParent();
@ -2196,7 +2196,7 @@ T* getTokenArgumentFunctionImpl(T* tok, int& argn)
parent = parent->astParent(); parent = parent->astParent();
// passing variable to subfunction? // passing variable to subfunction?
if (Token::Match(parent, "[[(,{]")) if (Token::Match(parent, "[*[(,{]"))
; ;
else if (Token::simpleMatch(parent, ":")) { else if (Token::simpleMatch(parent, ":")) {
while (Token::Match(parent, "[?:]")) while (Token::Match(parent, "[?:]"))
@ -2347,6 +2347,10 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti
if (addressOf) if (addressOf)
indirect++; indirect++;
const bool deref = tok->astParent() && tok->astParent()->isUnaryOp("*");
if (deref && indirect > 0)
indirect--;
int argnr; int argnr;
tok = getTokenArgumentFunction(tok, argnr); tok = getTokenArgumentFunction(tok, argnr);
if (!tok) if (!tok)
@ -2455,7 +2459,7 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings,
(Token::simpleMatch(tok2->astParent(), ":") && Token::simpleMatch(tok2->astParent()->astParent(), "?"))) (Token::simpleMatch(tok2->astParent(), ":") && Token::simpleMatch(tok2->astParent()->astParent(), "?")))
tok2 = tok2->astParent(); tok2 = tok2->astParent();
if (tok2->astParent() && tok2->astParent()->tokType() == Token::eIncDecOp) if (indirect == 0 && tok2->astParent() && tok2->astParent()->tokType() == Token::eIncDecOp)
return true; return true;
auto skipRedundantPtrOp = [](const Token* tok, const Token* parent) { auto skipRedundantPtrOp = [](const Token* tok, const Token* parent) {
@ -2578,7 +2582,7 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings,
const Token *parent = tok2->astParent(); const Token *parent = tok2->astParent();
while (Token::Match(parent, ".|::")) while (Token::Match(parent, ".|::"))
parent = parent->astParent(); parent = parent->astParent();
if (parent && parent->tokType() == Token::eIncDecOp) if (parent && parent->tokType() == Token::eIncDecOp && (indirect == 0 || tok2 != tok))
return true; return true;
// structured binding, nonconst reference variable in lhs // structured binding, nonconst reference variable in lhs
@ -2615,7 +2619,7 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings,
if (indirect > 0) { if (indirect > 0) {
// check for `*(ptr + 1) = new_value` case // check for `*(ptr + 1) = new_value` case
parent = tok2->astParent(); parent = tok2->astParent();
while (parent && parent->isArithmeticalOp() && parent->isBinaryOp()) { while (parent && ((parent->isArithmeticalOp() && parent->isBinaryOp()) || parent->isIncDecOp())) {
parent = parent->astParent(); parent = parent->astParent();
} }
if (Token::simpleMatch(parent, "*")) { if (Token::simpleMatch(parent, "*")) {
@ -2839,8 +2843,9 @@ bool isExpressionChanged(const Token* expr, const Token* start, const Token* end
if (vt->type == ValueType::ITERATOR) if (vt->type == ValueType::ITERATOR)
++indirect; ++indirect;
} }
if (isExpressionChangedAt(tok, tok2, indirect, global, settings, cpp, depth)) for (int i = 0; i <= indirect; ++i)
return true; if (isExpressionChangedAt(tok, tok2, i, global, settings, cpp, depth))
return true;
} }
} }
return false; return false;

13
lib/checkother.cpp
View File

@ -1587,9 +1587,10 @@ void CheckOther::checkConstPointer()
if (std::find(nonConstPointers.cbegin(), nonConstPointers.cend(), var) != nonConstPointers.cend()) if (std::find(nonConstPointers.cbegin(), nonConstPointers.cend(), var) != nonConstPointers.cend())
continue; continue;
pointers.emplace_back(var); pointers.emplace_back(var);
const Token* const parent = tok->astParent(); const Token* parent = tok->astParent();
enum Deref { NONE, DEREF, MEMBER } deref = NONE; enum Deref { NONE, DEREF, MEMBER } deref = NONE;
if (parent && parent->isUnaryOp("*")) bool hasIncDec = false;
if (parent && (parent->isUnaryOp("*") || (hasIncDec = parent->isIncDecOp() && parent->astParent() && parent->astParent()->isUnaryOp("*"))))
deref = DEREF; deref = DEREF;
else if (Token::simpleMatch(parent, "[") && parent->astOperand1() == tok && tok != nameTok) else if (Token::simpleMatch(parent, "[") && parent->astOperand1() == tok && tok != nameTok)
deref = DEREF; deref = DEREF;
@ -1600,7 +1601,7 @@ void CheckOther::checkConstPointer()
else if (astIsRangeBasedForDecl(tok)) else if (astIsRangeBasedForDecl(tok))
continue; continue;
if (deref != NONE) { if (deref != NONE) {
const Token* const gparent = parent->astParent(); const Token* gparent = parent->astParent();
if (deref == MEMBER) { if (deref == MEMBER) {
if (!gparent) if (!gparent)
continue; continue;
@ -1613,6 +1614,10 @@ void CheckOther::checkConstPointer()
} }
if (Token::Match(gparent, "%cop%") && !gparent->isUnaryOp("&") && !gparent->isUnaryOp("*")) if (Token::Match(gparent, "%cop%") && !gparent->isUnaryOp("&") && !gparent->isUnaryOp("*"))
continue; continue;
if (hasIncDec) {
parent = gparent;
gparent = gparent ? gparent->astParent() : nullptr;
}
int argn = -1; int argn = -1;
if (Token::simpleMatch(gparent, "return")) { if (Token::simpleMatch(gparent, "return")) {
const Function* function = gparent->scope()->function; const Function* function = gparent->scope()->function;
@ -1633,7 +1638,7 @@ void CheckOther::checkConstPointer()
continue; continue;
else if (const Token* ftok = getTokenArgumentFunction(parent, argn)) { else if (const Token* ftok = getTokenArgumentFunction(parent, argn)) {
bool inconclusive{}; bool inconclusive{};
if (!isVariableChangedByFunctionCall(ftok, vt->pointer, var->declarationId(), mSettings, &inconclusive) && !inconclusive) if (!isVariableChangedByFunctionCall(ftok->next(), vt->pointer, var->declarationId(), mSettings, &inconclusive) && !inconclusive)
continue; continue;
} }
} else { } else {

4
lib/checkunusedvar.cpp
View File

@ -1691,8 +1691,8 @@ bool CheckUnusedVar::isFunctionWithoutSideEffects(const Function& func, const To
} }
// check if global variable is changed // check if global variable is changed
if (bodyVariable->isGlobal() || (pointersToGlobals.find(bodyVariable) != pointersToGlobals.end())) { if (bodyVariable->isGlobal() || (pointersToGlobals.find(bodyVariable) != pointersToGlobals.end())) {
const int depth = 20; const int indirect = bodyVariable->isArray() ? bodyVariable->dimensions().size() : bodyVariable->isPointer();
if (isVariableChanged(bodyToken, depth, mSettings, mTokenizer->isCPP())) { if (isVariableChanged(bodyToken, indirect, mSettings, mTokenizer->isCPP())) {
return false; return false;
} }
// check if pointer to global variable assigned to another variable (another_var = &global_var) // check if pointer to global variable assigned to another variable (another_var = &global_var)

25
lib/valueflow.cpp
View File

@ -2564,19 +2564,22 @@ struct ValueFlowAnalyzer : Analyzer {
return read; return read;
} }
virtual Action isAliasModified(const Token* tok) const { virtual Action isAliasModified(const Token* tok, int indirect = -1) const {
// Lambda function call // Lambda function call
if (Token::Match(tok, "%var% (")) if (Token::Match(tok, "%var% ("))
// TODO: Check if modified in the lambda function // TODO: Check if modified in the lambda function
return Action::Invalid; return Action::Invalid;
int indirect = 0; if (indirect == -1) {
if (const ValueType* vt = tok->valueType()) { indirect = 0;
indirect = vt->pointer; if (const ValueType* vt = tok->valueType()) {
if (vt->type == ValueType::ITERATOR) indirect = vt->pointer;
++indirect; if (vt->type == ValueType::ITERATOR)
++indirect;
}
} }
if (isVariableChanged(tok, indirect, getSettings(), isCPP())) for (int i = 0; i <= indirect; ++i)
return Action::Invalid; if (isVariableChanged(tok, i, getSettings(), isCPP()))
return Action::Invalid;
return Action::None; return Action::None;
} }
@ -3213,6 +3216,12 @@ struct ExpressionAnalyzer : SingleValueFlowAnalyzer {
bool isVariable() const override { bool isVariable() const override {
return expr->varId() > 0; return expr->varId() > 0;
} }
Action isAliasModified(const Token* tok, int indirect) const override {
if (value.isSymbolicValue() && tok->exprId() == value.tokvalue->exprId())
indirect = 0;
return SingleValueFlowAnalyzer::isAliasModified(tok, indirect);
}
}; };
struct SameExpressionAnalyzer : ExpressionAnalyzer { struct SameExpressionAnalyzer : ExpressionAnalyzer {

8
test/testother.cpp
View File

@ -3800,6 +3800,12 @@ private:
" return strlen(p);\n" " return strlen(p);\n"
"}\n"); "}\n");
ASSERT_EQUALS("[test.cpp:1]: (style) Parameter 'p' can be declared as pointer to const\n", errout.str()); ASSERT_EQUALS("[test.cpp:1]: (style) Parameter 'p' can be declared as pointer to const\n", errout.str());
check("void f(int* p) {\n" // #11862
" long long j = *(p++);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:1]: (style) Parameter 'p' can be declared as pointer to const\n",
errout.str());
} }
void switchRedundantAssignmentTest() { void switchRedundantAssignmentTest() {
@ -8128,7 +8134,7 @@ private:
"void packed_object_info(struct object_info *oi) {\n" "void packed_object_info(struct object_info *oi) {\n"
" if (*oi->typep < 0);\n" " if (*oi->typep < 0);\n"
"}"); "}");
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (style) Parameter 'oi' can be declared as pointer to const\n", errout.str());
} }
void checkSuspiciousSemicolon1() { void checkSuspiciousSemicolon1() {

29
test/teststl.cpp
View File

@ -2819,27 +2819,20 @@ private:
} }
void eraseOnVector() { void eraseOnVector() {
check("void f(const std::vector<int>& m_ImplementationMap) {\n" check("void f(std::vector<int>& v) {\n"
" std::vector<int>::iterator aIt = m_ImplementationMap.begin();\n" " std::vector<int>::iterator aIt = v.begin();\n"
" m_ImplementationMap.erase(something(unknown));\n" // All iterators become invalidated when erasing from std::vector " v.erase(something(unknown));\n" // All iterators become invalidated when erasing from std::vector
" m_ImplementationMap.erase(aIt);\n" " v.erase(aIt);\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:4]: (error) Using iterator to local container 'm_ImplementationMap' that may be invalid.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:4]: (error) Using iterator to local container 'v' that may be invalid.\n", errout.str());
check("void f(const std::vector<int>& m_ImplementationMap) {\n" check("void f(std::vector<int>& v) {\n"
" std::vector<int>::iterator aIt = m_ImplementationMap.begin();\n" " std::vector<int>::iterator aIt = v.begin();\n"
" m_ImplementationMap.erase(*aIt);\n" // All iterators become invalidated when erasing from std::vector " std::vector<int>::iterator bIt = v.begin();\n"
" m_ImplementationMap.erase(aIt);\n" " v.erase(bIt);\n" // All iterators become invalidated when erasing from std::vector
" aIt = v.erase(aIt);\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:4]: (error) Using iterator to local container 'm_ImplementationMap' that may be invalid.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2] -> [test.cpp:4] -> [test.cpp:1] -> [test.cpp:5]: (error) Using iterator to local container 'v' that may be invalid.\n", errout.str());
check("void f(const std::vector<int>& m_ImplementationMap) {\n"
" std::vector<int>::iterator aIt = m_ImplementationMap.begin();\n"
" std::vector<int>::iterator bIt = m_ImplementationMap.begin();\n"
" m_ImplementationMap.erase(*bIt);\n" // All iterators become invalidated when erasing from std::vector
" aIt = m_ImplementationMap.erase(aIt);\n"
"}");
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2] -> [test.cpp:4] -> [test.cpp:1] -> [test.cpp:5]: (error) Using iterator to local container 'm_ImplementationMap' that may be invalid.\n", errout.str());
} }
void pushback1() { void pushback1() {