From 78182d477315b9c8d6387bb08373c96030348c15 Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Mon, 22 May 2023 19:53:51 +0200 Subject: [PATCH] Fix FN constVariablePointer (#5076) * Fix FN constVariablePointer * Fix FP * Add const * Fix tests * Add const --- cli/cmdlineparser.cpp | 10 +++++----- gui/librarydialog.cpp | 2 +- lib/astutils.cpp | 2 +- lib/checkautovariables.cpp | 2 +- lib/checkother.cpp | 14 +++++++++++++- lib/checkunusedvar.cpp | 2 +- lib/clangimport.cpp | 6 +++--- lib/reverseanalyzer.cpp | 2 +- lib/templatesimplifier.cpp | 4 ++-- lib/tokenize.cpp | 4 ++-- lib/valueflow.cpp | 5 ++--- test/cfg/bsd.c | 1 + test/cfg/gnu.c | 5 +++-- test/cfg/posix.c | 16 ++++++---------- test/cfg/std.c | 9 ++++++--- test/cfg/windows.cpp | 3 +-- test/testother.cpp | 33 +++++++++++++++++++++++++-------- tools/triage/mainwindow.cpp | 2 +- 18 files changed, 75 insertions(+), 47 deletions(-) diff --git a/cli/cmdlineparser.cpp b/cli/cmdlineparser.cpp index 96390367a..32512efc3 100644 --- a/cli/cmdlineparser.cpp +++ b/cli/cmdlineparser.cpp @@ -791,26 +791,26 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[]) for (; node && strcmp(node->Value(), "rule") == 0; node = node->NextSiblingElement()) { Settings::Rule rule; - tinyxml2::XMLElement *tokenlist = node->FirstChildElement("tokenlist"); + const tinyxml2::XMLElement *tokenlist = node->FirstChildElement("tokenlist"); if (tokenlist) rule.tokenlist = tokenlist->GetText(); - tinyxml2::XMLElement *pattern = node->FirstChildElement("pattern"); + const tinyxml2::XMLElement *pattern = node->FirstChildElement("pattern"); if (pattern) { rule.pattern = pattern->GetText(); } tinyxml2::XMLElement *message = node->FirstChildElement("message"); if (message) { - tinyxml2::XMLElement *severity = message->FirstChildElement("severity"); + const tinyxml2::XMLElement *severity = message->FirstChildElement("severity"); if (severity) rule.severity = Severity::fromString(severity->GetText()); - tinyxml2::XMLElement *id = message->FirstChildElement("id"); + const tinyxml2::XMLElement *id = message->FirstChildElement("id"); if (id) rule.id = id->GetText(); - tinyxml2::XMLElement *summary = message->FirstChildElement("summary"); + const tinyxml2::XMLElement *summary = message->FirstChildElement("summary"); if (summary) rule.summary = summary->GetText() ? summary->GetText() : ""; } diff --git a/gui/librarydialog.cpp b/gui/librarydialog.cpp index 2e3cd0229..d258e59c0 100644 --- a/gui/librarydialog.cpp +++ b/gui/librarydialog.cpp @@ -275,7 +275,7 @@ void LibraryDialog::sortFunctions(bool sort) mUi->functions->sortItems(); } else { mIgnoreChanges = true; - CppcheckLibraryData::Function *selfunction = currentFunction(); + const CppcheckLibraryData::Function* selfunction = currentFunction(); mUi->functions->clear(); for (CppcheckLibraryData::Function &function : mData.functions) { mUi->functions->addItem(new FunctionListItem(mUi->functions, diff --git a/lib/astutils.cpp b/lib/astutils.cpp index dfe93db95..d247bd29f 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -2632,7 +2632,7 @@ bool isVariableChanged(const Token *start, const Token *end, int indirect, const const Token* findExpression(const Token* start, const nonneg int exprid) { - Function * f = Scope::nestedInFunction(start->scope()); + const Function* f = Scope::nestedInFunction(start->scope()); if (!f) return nullptr; const Scope* scope = f->functionScope; diff --git a/lib/checkautovariables.cpp b/lib/checkautovariables.cpp index 1cc494b88..b6f6ca7ac 100644 --- a/lib/checkautovariables.cpp +++ b/lib/checkautovariables.cpp @@ -475,7 +475,7 @@ static bool isDanglingSubFunction(const Token* tokvalue, const Token* tok) const Variable* var = tokvalue->variable(); if (!var->isLocal()) return false; - Function* f = Scope::nestedInFunction(tok->scope()); + const Function* f = Scope::nestedInFunction(tok->scope()); if (!f) return false; const Token* parent = tokvalue->astParent(); diff --git a/lib/checkother.cpp b/lib/checkother.cpp index b82df24e9..9f6334771 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1537,6 +1537,16 @@ void CheckOther::checkConstVariable() } } +static const Token* getVariableChangedStart(const Variable* p) +{ + if (p->isArgument()) + return p->scope()->bodyStart; + const Token* start = p->nameToken()->next(); + if (start->isSplittedVarDeclEq()) + start = start->tokAt(3); + return start; +} + void CheckOther::checkConstPointer() { if (!mSettings->severity.isEnabled(Severity::style)) @@ -1619,6 +1629,8 @@ void CheckOther::checkConstPointer() continue; else if (Token::simpleMatch(parent, "(") && Token::Match(parent->astOperand1(), "if|while")) continue; + else if (Token::simpleMatch(parent, "=") && parent->astOperand1() == tok) + continue; else if (const Token* ftok = getTokenArgumentFunction(tok, argn)) { if (ftok->function() && !parent->isCast()) { const Variable* argVar = ftok->function()->getArgumentVar(argn); @@ -1638,7 +1650,7 @@ void CheckOther::checkConstPointer() continue; } if (std::find(nonConstPointers.cbegin(), nonConstPointers.cend(), p) == nonConstPointers.cend()) { - const Token *start = (p->isArgument()) ? p->scope()->bodyStart : p->nameToken()->next(); + const Token *start = getVariableChangedStart(p); const int indirect = p->isArray() ? p->dimensions().size() : 1; if (isVariableChanged(start, p->scope()->bodyEnd, indirect, p->declarationId(), false, mSettings, mTokenizer->isCPP())) continue; diff --git a/lib/checkunusedvar.cpp b/lib/checkunusedvar.cpp index 0bf9ca3d0..fd98fd68c 100644 --- a/lib/checkunusedvar.cpp +++ b/lib/checkunusedvar.cpp @@ -293,7 +293,7 @@ void Variables::read(nonneg int varid, const Token* tok) void Variables::readAliases(nonneg int varid, const Token* tok) { - VariableUsage *usage = find(varid); + const VariableUsage *usage = find(varid); if (usage) { for (nonneg int const aliases : usage->_aliases) { diff --git a/lib/clangimport.cpp b/lib/clangimport.cpp index a57057660..b5d9b8f41 100644 --- a/lib/clangimport.cpp +++ b/lib/clangimport.cpp @@ -1149,10 +1149,10 @@ Token *clangimport::AstNode::createTokens(TokenList *tokenList) if (nodeType == NamespaceDecl) { if (children.empty()) return nullptr; - Token *defToken = addtoken(tokenList, "namespace"); + const Token *defToken = addtoken(tokenList, "namespace"); const std::string &s = mExtTokens[mExtTokens.size() - 2]; - Token *nameToken = (s.compare(0,4,"col:")==0 || s.compare(0,5,"line:")==0) ? - addtoken(tokenList, mExtTokens.back()) : nullptr; + const Token* nameToken = (s.compare(0, 4, "col:") == 0 || s.compare(0, 5, "line:") == 0) ? + addtoken(tokenList, mExtTokens.back()) : nullptr; Scope *scope = createScope(tokenList, Scope::ScopeType::eNamespace, children, defToken); if (nameToken) scope->className = nameToken->str(); diff --git a/lib/reverseanalyzer.cpp b/lib/reverseanalyzer.cpp index af5942008..70311afbc 100644 --- a/lib/reverseanalyzer.cpp +++ b/lib/reverseanalyzer.cpp @@ -162,7 +162,7 @@ struct ReverseTraversal { parent = parent->astParent(); if (!Token::Match(parent, "%oror%|&&|?")) continue; - Token* condTok = parent->astOperand1(); + const Token* condTok = parent->astOperand1(); if (!condTok) continue; bool checkThen, checkElse; diff --git a/lib/templatesimplifier.cpp b/lib/templatesimplifier.cpp index 0beb610d1..267ac15dd 100644 --- a/lib/templatesimplifier.cpp +++ b/lib/templatesimplifier.cpp @@ -1320,7 +1320,7 @@ void TemplateSimplifier::simplifyTemplateAliases() // copy template-id from declaration to after instantiation Token * dst = aliasUsage.token()->next()->findClosingBracket(); - Token * end = TokenList::copyTokens(dst, aliasDeclaration.aliasStartToken(), aliasDeclaration.aliasEndToken()->previous(), false)->next(); + const Token* end = TokenList::copyTokens(dst, aliasDeclaration.aliasStartToken(), aliasDeclaration.aliasEndToken()->previous(), false)->next(); // replace parameters for (Token *tok1 = dst->next(); tok1 != end; tok1 = tok1->next()) { @@ -3740,7 +3740,7 @@ void TemplateSimplifier::simplifyTemplates( for (Token *tok = mTokenList.front(); tok; tok = tok->next()) { if (!Token::Match(tok, ")|>|>> requires %name%|(")) continue; - Token *end = skipRequires(tok->next()); + const Token* end = skipRequires(tok->next()); if (end) Token::eraseTokens(tok, end); } diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp index 6b1f62e25..163fef002 100644 --- a/lib/tokenize.cpp +++ b/lib/tokenize.cpp @@ -892,7 +892,7 @@ namespace { if (!after) throw InternalError(tok, "Failed to simplify typedef. Is the code valid?"); - Token* const tok4 = useAfterVarRange ? insertTokens(after->previous(), mRangeAfterVar)->next() : tok3->next(); + const Token* const tok4 = useAfterVarRange ? insertTokens(after->previous(), mRangeAfterVar)->next() : tok3->next(); tok->deleteThis(); @@ -4287,7 +4287,7 @@ void Tokenizer::setVarIdClassDeclaration(Token* const startToken, std::map>& structMembers) { // end of scope - Token* const endToken = startToken->link(); + const Token* const endToken = startToken->link(); // determine class name std::string className; diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 80acc30b2..51c995eef 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -6276,7 +6276,7 @@ struct ConditionHandler { continue; } if (Token::Match(tok->astParent(), "==|!=")) { - Token* sibling = tok->astSibling(); + const Token* sibling = tok->astSibling(); if (sibling->hasKnownIntValue() && (astIsBool(tok) || astIsBool(sibling))) { const bool value = sibling->values().front().intvalue; if (inverted) @@ -7354,8 +7354,7 @@ static void valueFlowInjectParameter(TokenList* tokenlist, }); if (!r) { std::string fname = ""; - Function* f = functionScope->function; - if (f) + if (const Function* f = functionScope->function) fname = f->name(); if (settings.debugwarnings) bailout(tokenlist, errorLogger, functionScope->bodyStart, "Too many argument passed to " + fname); diff --git a/test/cfg/bsd.c b/test/cfg/bsd.c index 3c8d2e688..227e327c9 100644 --- a/test/cfg/bsd.c +++ b/test/cfg/bsd.c @@ -77,6 +77,7 @@ void uninitvar_timercmp(struct timeval t) void nullPointer_timercmp(struct timeval t) { + // cppcheck-suppress constVariablePointer struct timeval *p=0; // cppcheck-suppress nullPointer (void)timercmp(&t, p, <); diff --git a/test/cfg/gnu.c b/test/cfg/gnu.c index 2ca230a90..66e74acf4 100644 --- a/test/cfg/gnu.c +++ b/test/cfg/gnu.c @@ -205,6 +205,7 @@ void uninitvar_timercmp(struct timeval t) void nullPointer_timercmp(struct timeval t) { + // cppcheck-suppress constVariablePointer struct timeval *p=0; // cppcheck-suppress nullPointer (void)timercmp(&t, p, <); @@ -417,11 +418,11 @@ void bufferAccessOutOfBounds() void leakReturnValNotUsed() { - // cppcheck-suppress unreadVariable + // cppcheck-suppress [unreadVariable, constVariablePointer] char* ptr = (char*)strdupa("test"); // cppcheck-suppress ignoredReturnValue strdupa("test"); - // cppcheck-suppress unreadVariable + // cppcheck-suppress [unreadVariable, constVariablePointer] char* ptr2 = (char*)strndupa("test", 1); // cppcheck-suppress ignoredReturnValue strndupa("test", 1); diff --git a/test/cfg/posix.c b/test/cfg/posix.c index 51150b568..fc20159e2 100644 --- a/test/cfg/posix.c +++ b/test/cfg/posix.c @@ -985,8 +985,7 @@ void memleak_getaddrinfo() void memleak_mmap(int fd) { - // cppcheck-suppress unusedAllocatedMemory - // cppcheck-suppress unreadVariable + // cppcheck-suppress [unusedAllocatedMemory, unreadVariable, constVariablePointer] void *addr = mmap(NULL, 255, PROT_NONE, MAP_PRIVATE, fd, 0); // cppcheck-suppress memleak } @@ -1040,7 +1039,7 @@ int munmap_no_double_free(int tofd, // #11396 void resourceLeak_fdopen(int fd) { - // cppcheck-suppress unreadVariable + // cppcheck-suppress [unreadVariable, constVariablePointer] FILE *f = fdopen(fd, "r"); // cppcheck-suppress resourceLeak } @@ -1065,14 +1064,14 @@ int no_resourceLeak_mkstemp_02(char *template) void resourceLeak_fdopendir(int fd) { - // cppcheck-suppress unreadVariable + // cppcheck-suppress [unreadVariable, constVariablePointer] DIR* leak1 = fdopendir(fd); // cppcheck-suppress resourceLeak } void resourceLeak_opendir(void) { - // cppcheck-suppress unreadVariable + // cppcheck-suppress [unreadVariable, constVariablePointer] DIR* leak1 = opendir("abc"); // cppcheck-suppress resourceLeak } @@ -1187,9 +1186,7 @@ void uninitvar(int fd) regcomp(®, pattern, cflags2); regerror(0, ®, 0, 0); #ifndef __CYGWIN__ - // cppcheck-suppress uninitvar - // cppcheck-suppress unreadVariable - // cppcheck-suppress ecvtCalled + // cppcheck-suppress [uninitvar, unreadVariable, ecvtCalled, constVariablePointer] char *buffer = ecvt(d, 11, &decimal, &sign); #endif // cppcheck-suppress gcvtCalled @@ -1282,8 +1279,7 @@ void dl(const char* libname, const char* func) // cppcheck-suppress resourceLeak lib = dlopen(libname, RTLD_LAZY); const char* funcname; - // cppcheck-suppress uninitvar - // cppcheck-suppress unreadVariable + // cppcheck-suppress [uninitvar, unreadVariable, constVariablePointer] void* sym = dlsym(lib, funcname); // cppcheck-suppress ignoredReturnValue dlsym(lib, "foo"); diff --git a/test/cfg/std.c b/test/cfg/std.c index a2183b17d..1cc16fefc 100644 --- a/test/cfg/std.c +++ b/test/cfg/std.c @@ -264,8 +264,7 @@ char* nullPointer_fgets(char *buffer, int n, FILE *stream) void memleak_aligned_alloc(void) { - // cppcheck-suppress unusedAllocatedMemory - // cppcheck-suppress unreadVariable + // cppcheck-suppress [unusedAllocatedMemory, unreadVariable, constVariablePointer] char * alignedBuf = aligned_alloc(8, 16); // cppcheck-suppress memleak } @@ -358,7 +357,7 @@ void arrayIndexOutOfBounds() void resourceLeak_tmpfile(void) { - // cppcheck-suppress unreadVariable + // cppcheck-suppress [unreadVariable, constVariablePointer] FILE * fp = tmpfile(); // cppcheck-suppress resourceLeak } @@ -4184,6 +4183,7 @@ void uninitvar_tolower(int character) // cppcheck-suppress unassignedVariable int c2; + // cppcheck-suppress constVariablePointer int *pc=&c2; // cppcheck-suppress uninitvar (void)tolower(*pc); @@ -4191,6 +4191,7 @@ void uninitvar_tolower(int character) // No warning is expected (void)tolower(character); + // cppcheck-suppress constVariablePointer int *pChar = &character; // No warning is expected (void)tolower(*pChar); @@ -4204,6 +4205,7 @@ void uninitvar_toupper(int character) // cppcheck-suppress unassignedVariable int c2; + // cppcheck-suppress constVariablePointer int *pc=&c2; // cppcheck-suppress uninitvar (void)toupper(*pc); @@ -4211,6 +4213,7 @@ void uninitvar_toupper(int character) // No warning is expected (void)toupper(character); + // cppcheck-suppress constVariablePointer int *pChar = &character; // No warning is expected (void)toupper(*pChar); diff --git a/test/cfg/windows.cpp b/test/cfg/windows.cpp index 45039f9b3..a9ceedc4e 100644 --- a/test/cfg/windows.cpp +++ b/test/cfg/windows.cpp @@ -525,8 +525,7 @@ void nullPointer() void memleak_malloca() { - // cppcheck-suppress unusedAllocatedMemory - // cppcheck-suppress unreadVariable + // cppcheck-suppress [unusedAllocatedMemory, unreadVariable, constVariablePointer] void *pMem = _malloca(10); // cppcheck-suppress memleak } diff --git a/test/testother.cpp b/test/testother.cpp index f9e58b8ef..4ba61960b 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3632,6 +3632,11 @@ private: "[test.cpp:13]: (style) Parameter 's' can be declared as pointer to const\n" "[test.cpp:19]: (style) Parameter 's' can be declared as pointer to const\n", errout.str()); + + check("void f(int*& p, int* q) {\n" + " p = q;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void switchRedundantAssignmentTest() { @@ -7440,7 +7445,7 @@ private: " if (*p < 0) continue;\n" " if ((*p > 0)) {}\n" "}\n"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style) Variable 'p' can be declared as pointer to const\n", errout.str()); check("void f() {\n" " int val = 0;\n" @@ -7449,7 +7454,9 @@ private: " if ((*p > 0)) {}\n" "}\n"); TODO_ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The comparison '*p < 0' is always false.\n" - "[test.cpp:2] -> [test.cpp:4]: (style) The comparison '*p > 0' is always false.\n", "", errout.str()); + "[test.cpp:2] -> [test.cpp:4]: (style) The comparison '*p > 0' is always false.\n", + "[test.cpp:3]: (style) Variable 'p' can be declared as pointer to const\n", + errout.str()); check("void f() {\n" " int val = 0;\n" @@ -9747,7 +9754,7 @@ private: check("void f(char **ptr) {\n" " int *x = &(*ptr)[10];\n" "}\n", nullptr, true); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Variable 'x' can be declared as pointer to const\n", errout.str()); // function calls check("void f(Mutex *mut) {\n" @@ -10887,7 +10894,9 @@ private: " return xp > yp;\n" "}"); ASSERT_EQUALS( - "[test.cpp:2] -> [test.cpp:4] -> [test.cpp:3] -> [test.cpp:5] -> [test.cpp:6]: (error) Comparing pointers that point to different objects\n", + "[test.cpp:2] -> [test.cpp:4] -> [test.cpp:3] -> [test.cpp:5] -> [test.cpp:6]: (error) Comparing pointers that point to different objects\n" + "[test.cpp:4]: (style) Variable 'xp' can be declared as pointer to const\n" + "[test.cpp:5]: (style) Variable 'yp' can be declared as pointer to const\n", errout.str()); check("bool f() {\n" @@ -10908,7 +10917,9 @@ private: " return xp > yp;\n" "}"); ASSERT_EQUALS( - "[test.cpp:1] -> [test.cpp:5] -> [test.cpp:1] -> [test.cpp:6] -> [test.cpp:7]: (error) Comparing pointers that point to different objects\n", + "[test.cpp:1] -> [test.cpp:5] -> [test.cpp:1] -> [test.cpp:6] -> [test.cpp:7]: (error) Comparing pointers that point to different objects\n" + "[test.cpp:5]: (style) Variable 'xp' can be declared as pointer to const\n" + "[test.cpp:6]: (style) Variable 'yp' can be declared as pointer to const\n", errout.str()); check("struct A {int data;};\n" @@ -10920,7 +10931,9 @@ private: " return xp > yp;\n" "}"); ASSERT_EQUALS( - "[test.cpp:2] -> [test.cpp:3] -> [test.cpp:5] -> [test.cpp:2] -> [test.cpp:4] -> [test.cpp:6] -> [test.cpp:7]: (error) Comparing pointers that point to different objects\n", + "[test.cpp:2] -> [test.cpp:3] -> [test.cpp:5] -> [test.cpp:2] -> [test.cpp:4] -> [test.cpp:6] -> [test.cpp:7]: (error) Comparing pointers that point to different objects\n" + "[test.cpp:5]: (style) Variable 'xp' can be declared as pointer to const\n" + "[test.cpp:6]: (style) Variable 'yp' can be declared as pointer to const\n", errout.str()); check("bool f(int * xp, int* yp) {\n" @@ -10945,7 +10958,9 @@ private: " int* yp = &x[1];\n" " return xp > yp;\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style) Variable 'xp' can be declared as pointer to const\n" + "[test.cpp:4]: (style) Variable 'yp' can be declared as pointer to const\n", + errout.str()); check("bool f(const int * xp, const int* yp) {\n" " return xp > yp;\n" @@ -10975,7 +10990,9 @@ private: " int* yp = &y->data;\n" " return xp > yp;\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (style) Variable 'xp' can be declared as pointer to const\n" + "[test.cpp:6]: (style) Variable 'yp' can be declared as pointer to const\n", + errout.str()); check("struct S { int i; };\n" // #11576 "int f(S s) {\n" diff --git a/tools/triage/mainwindow.cpp b/tools/triage/mainwindow.cpp index 45be8c054..d816bc005 100644 --- a/tools/triage/mainwindow.cpp +++ b/tools/triage/mainwindow.cpp @@ -412,7 +412,7 @@ void MainWindow::resultsContextMenu(const QPoint& pos) return; QMenu submenu; submenu.addAction("Copy"); - QAction* menuItem = submenu.exec(ui->results->mapToGlobal(pos)); + const QAction* menuItem = submenu.exec(ui->results->mapToGlobal(pos)); if (menuItem && menuItem->text().contains("Copy")) { QString text;