Skip to content

Move some checks for variable changed from constVariable check to isVariableChanged #4905

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
firewave merged 6 commits into from
Mar 21, 2023
Merged

Move some checks for variable changed from constVariable check to isVariableChanged #4905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
827f6a8
Move checks for variable changed from constVariable check to isVariab…
pfultz2 Mar 21, 2023
3371e15
Format
pfultz2 Mar 21, 2023
b472aee
Add test cases
pfultz2 Mar 21, 2023
bd0d601
Format
pfultz2 Mar 21, 2023
d804b74
Fix windows error
pfultz2 Mar 21, 2023
0c31c22
Format
pfultz2 Mar 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions gui/threadresult.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,9 @@ void ThreadResult::setFiles(const QStringList &files)

// Determine the total size of all of the files to check, so that we can
// show an accurate progress estimate
quint64 sizeOfFiles = 0;
for (const QString& file : files) {
sizeOfFiles += QFile(file).size();
}
quint64 sizeOfFiles = std::accumulate(files.begin(), files.end(), 0, [](quint64 total, const QString& file) {
return total + QFile(file).size();
});
mMaxProgress = sizeOfFiles;
}

Expand Down
45 changes: 31 additions & 14 deletions lib/astutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2477,29 +2477,35 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings,
}
}

// Check addressof
if (tok2->astParent() && tok2->astParent()->isUnaryOp("&") &&
isVariableChanged(tok2->astParent(), indirect + 1, settings, depth - 1)) {
return true;
}

const ValueType* vt = tok->variable() ? tok->variable()->valueType() : tok->valueType();
// If its already const then it cant be modified
if (vt) {
if (vt->constness & (1 << indirect))
return false;
}

if (cpp && Token::Match(tok2->astParent(), ">>|&") && astIsRHS(tok2) && isLikelyStreamRead(cpp, tok2->astParent()))
return true;

if (isLikelyStream(cpp, tok2))
return true;

// Member function call
if (tok->variable() && Token::Match(tok2->astParent(), ". %name%") && isFunctionCall(tok2->astParent()->next()) && tok2->astParent()->astOperand1() == tok2) {
const Variable * var = tok->variable();
if (Token::Match(tok2->astParent(), ". %name%") && isFunctionCall(tok2->astParent()->next()) &&
tok2->astParent()->astOperand1() == tok2) {
// Member function cannot change what `this` points to
if (indirect == 0 && astIsPointer(tok))
return false;
bool isConst = var && var->isConst();
if (!isConst) {
const ValueType * valueType = var->valueType();
isConst = (valueType && valueType->pointer == 1 && valueType->constness == 1);
}
if (isConst)
return false;

const Token *ftok = tok->tokAt(2);
if (astIsContainer(tok) && tok->valueType() && tok->valueType()->container) {
const Library::Container* c = tok->valueType()->container;
if (astIsContainer(tok) && vt && vt->container) {
const Library::Container* c = vt->container;
const Library::Container::Action action = c->getAction(ftok->str());
if (contains({Library::Container::Action::INSERT,
Library::Container::Action::ERASE,
Expand Down Expand Up @@ -2529,15 +2535,26 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings,
return false;
}
}
if (settings)
return !settings->library.isFunctionConst(ftok);
if (settings && settings->library.isFunctionConst(ftok))
return false;

const Function * fun = ftok->function();
if (!fun)
return true;
return !fun->isConst();
}

// Member pointer
if (Token::Match(tok2->astParent(), ". * ( & %name% ::")) {
const Token* ftok = tok2->astParent()->linkAt(2)->previous();
// TODO: Check for pointer to member variable
if (!ftok->function() || !ftok->function()->isConst())
return true;
}

if (Token::simpleMatch(tok2, "[") && astIsContainer(tok) && vt && vt->container && vt->container->stdAssociativeLike)
return true;

const Token *ftok = tok2;
while (ftok && (!Token::Match(ftok, "[({]") || ftok->isCast()))
ftok = ftok->astParent();
Expand Down Expand Up @@ -2675,7 +2692,7 @@ static bool isExpressionChangedAt(const F& getExprTok,
aliased = isAliasOf(tok, expr);
if (!aliased)
return false;
if (isVariableChanged(tok, 1, settings, cpp, depth))
if (isVariableChanged(tok, indirect + 1, settings, cpp, depth))
return true;
// TODO: Try to traverse the lambda function
if (Token::Match(tok, "%var% ("))
Expand Down
12 changes: 6 additions & 6 deletions lib/astutils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,12 +337,12 @@ bool isThisChanged(const Token* start, const Token* end, int indirect, const Set
const Token* findVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int exprid, bool globalvar, const Settings *settings, bool cpp, int depth = 20);
Token* findVariableChanged(Token *start, const Token *end, int indirect, const nonneg int exprid, bool globalvar, const Settings *settings, bool cpp, int depth = 20);

bool isExpressionChanged(const Token* expr,
const Token* start,
const Token* end,
const Settings* settings,
bool cpp,
int depth = 20);
CPPCHECKLIB bool isExpressionChanged(const Token* expr,
const Token* start,
const Token* end,
const Settings* settings,
bool cpp,
int depth = 20);

bool isExpressionChangedAt(const Token* expr,
const Token* tok,
Expand Down
43 changes: 0 additions & 43 deletions lib/checkother.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1489,49 +1489,6 @@ void CheckOther::checkConstVariable()
if (castToNonConst)
continue;
}
// Do not warn if struct data is changed
{
bool changeStructData = false;
for (const Token* tok = var->nameToken(); tok != scope->bodyEnd && tok != nullptr; tok = tok->next()) {
if (tok->variable() == var && Token::Match(tok, "%var% .")) {
const Token *parent = tok;
while (Token::simpleMatch(parent->astParent(), ".") && parent == parent->astParent()->astOperand1())
parent = parent->astParent();
if (parent->valueType() &&
parent->valueType()->pointer > 0 &&
parent->valueType()->constness == 0 &&
isVariableChanged(parent, 1, mSettings, mTokenizer->isCPP())) {
changeStructData = true;
break;
}
}
}
if (changeStructData)
continue;
}
// Calling non-const method using non-const reference
if (var->isReference()) {
bool callNonConstMethod = false;
for (const Token* tok = var->nameToken(); tok != scope->bodyEnd && tok != nullptr; tok = tok->next()) {
if (tok->variable() == var) {
if (Token::Match(tok, "%var% . * ( & %name% ::")) {
const Token* ftok = tok->linkAt(3)->previous();
if (!ftok->function() || !ftok->function()->isConst())
callNonConstMethod = true;
break;
}
if (var->valueType() && var->valueType()->container && Token::Match(tok, "%var% [")) { // containers whose operator[] is non-const
const auto& notConst = CheckClass::stl_containers_not_const;
if (notConst.find(var->valueType()->container->startPattern) != notConst.end()) {
callNonConstMethod = true;
break;
}
}
}
}
if (callNonConstMethod)
continue;
}

constVariableError(var, hasFunction ? function : nullptr);
}
Expand Down
13 changes: 7 additions & 6 deletions test/fixture.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,13 @@ extern std::ostringstream output;
#define EXPECT_EQ( EXPECTED, ACTUAL ) assertEquals(__FILE__, __LINE__, EXPECTED, ACTUAL)
#define REGISTER_TEST( CLASSNAME ) namespace { CLASSNAME instance_ ## CLASSNAME; }

#define LOAD_LIB_2( LIB, NAME ) do { \
if (((LIB).load(exename.c_str(), NAME).errorcode != Library::ErrorCode::OK)) { \
complainMissingLib(NAME); \
return; \
} \
} while (false)
#define LOAD_LIB_2(LIB, NAME) \
do { \
if (((LIB).load(exename.c_str(), NAME).errorcode != Library::ErrorCode::OK)) { \
complainMissingLib(NAME); \
abort(); \
} \
} while (false)

#define PLATFORM( P, T ) do { std::string errstr; assertEquals(__FILE__, __LINE__, true, P.set(cppcheck::Platform::toString(T), errstr, {exename}), errstr); } while (false)

Expand Down
48 changes: 48 additions & 0 deletions test/testastutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class TestAstUtils : public TestFixture {
TEST_CASE(isSameExpressionTest);
TEST_CASE(isVariableChangedTest);
TEST_CASE(isVariableChangedByFunctionCallTest);
TEST_CASE(isExpressionChangedTest);
TEST_CASE(nextAfterAstRightmostLeafTest);
TEST_CASE(isUsedAsBool);
}
Expand Down Expand Up @@ -227,6 +228,13 @@ class TestAstUtils : public TestFixture {
ASSERT_EQUALS(true, isVariableChanged("void f() {\n"
" int &a = a;\n"
"}\n", "= a", "}"));

ASSERT_EQUALS(false, isVariableChanged("void f(const A& a) { a.f(); }", "{", "}"));
ASSERT_EQUALS(true,
isVariableChanged("void g(int*);\n"
"void f(int x) { g(&x); }\n",
"{",
"}"));
}

#define isVariableChangedByFunctionCall(code, pattern, inconclusive) isVariableChangedByFunctionCall_(code, pattern, inconclusive, __FILE__, __LINE__)
Expand Down Expand Up @@ -258,6 +266,46 @@ class TestAstUtils : public TestFixture {
TODO_ASSERT_EQUALS(false, true, inconclusive);
}

#define isExpressionChanged(code, var, startPattern, endPattern) \
isExpressionChanged_(code, var, startPattern, endPattern, __FILE__, __LINE__)
bool isExpressionChanged_(const char code[],
const char var[],
const char startPattern[],
const char endPattern[],
const char* file,
int line)
{
Settings settings;
LOAD_LIB_2(settings.library, "std.cfg");
Tokenizer tokenizer(&settings, this);
std::istringstream istr(code);
ASSERT_LOC(tokenizer.tokenize(istr, "test.cpp"), file, line);
const Token* const start = Token::findsimplematch(tokenizer.tokens(), startPattern, strlen(startPattern));
const Token* const end = Token::findsimplematch(start, endPattern, strlen(endPattern));
const Token* const expr = Token::findsimplematch(tokenizer.tokens(), var, strlen(var));
return (isExpressionChanged)(expr, start, end, &settings, true);
}

void isExpressionChangedTest()
{
ASSERT_EQUALS(true, isExpressionChanged("void f(std::map<int, int>& m) { m[0]; }", "m [", "{", "}"));
ASSERT_EQUALS(false, isExpressionChanged("void f(const A& a) { a.f(); }", "a .", "{", "}"));

ASSERT_EQUALS(true,
isExpressionChanged("void g(int*);\n"
"void f(int x) { g(&x); }\n",
"x",
") {",
"}"));

ASSERT_EQUALS(true,
isExpressionChanged("struct S { void f(); int i; };\n"
"void call_f(S& s) { (s.*(&S::f))(); }\n",
"s .",
"{ (",
"}"));
}

#define nextAfterAstRightmostLeaf(code, parentPattern, rightPattern) nextAfterAstRightmostLeaf_(code, parentPattern, rightPattern, __FILE__, __LINE__)
bool nextAfterAstRightmostLeaf_(const char code[], const char parentPattern[], const char rightPattern[], const char* file, int line) {
Settings settings;
Expand Down