Skip to content

ProgramMemory: avoid duplicated lookups / removed at()#7768

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

Open
firewave wants to merge 2 commits into
base:main
Choose a base branch
from
Open

ProgramMemory: avoid duplicated lookups / removed at()#7768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c1dd20
ProgramMemory: consistently added `impossible` parameter to lookup fu…
firewave Sep 15, 2025
1328408
ProgramMemory: avoid duplicated lookups / removed `at()`
firewave Aug 25, 2025
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
143 changes: 75 additions & 68 deletions lib/programmemory.cpp
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -96,9 +96,24 @@ const ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossib
return nullptr;
}

bool ProgramMemory::getIntValue(nonneg int exprid, MathLib::bigint& result) const
ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossible)

@pfultz2pfultz2Sep 16, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should return a const ValueFlow::Value*. We dont ever mutate the ValueFlow::Value directly.

@firewavefirewaveSep 16, 2025

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do. This was previously done from the non-const at().

{
const ValueFlow::Value* value = getValue(exprid);
if (mValues->empty())
return nullptr;

// TODO: avoid copy if no value is found
copyOnWrite();

const auto it = find(exprid);
const bool found = it != mValues->cend() && (impossible || !it->second.isImpossible());

@pfultz2pfultz2Sep 16, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isImpossible can be checked by the consumers.

@firewavefirewaveSep 16, 2025

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about doing that.

@firewavefirewaveSep 17, 2025

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh - it is already handled inside some of the getters. So it was even more inconsistent as it seemed to be. They also return the underlying MathLib::bigint so the impossible handling cannot just be externalized...

@firewavefirewaveNov 12, 2025

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the impossible handling cannot be easily moved out I just rebased it so it can be merged.

@danmardanmarApr 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does it mean if impossible is true. That the caller only wants impossible value? If so we should write:

Suggested change
constbool found = it != mValues->cend() && (impossible || !it->second.isImpossible());
constbool found = it != mValues->cend() && (impossible == it->second.isImpossible());

if (found)
return &it->second;
return nullptr;
}

bool ProgramMemory::getIntValue(nonneg int exprid, MathLib::bigint& result, bool impossible) const
{
const ValueFlow::Value* value = getValue(exprid, impossible);
if (value && value->isIntValue()) {
result = value->intvalue;
return true;
Expand All@@ -114,9 +129,9 @@ void ProgramMemory::setIntValue(const Token* expr, MathLib::bigint value, bool i
setValue(expr, v);
}

bool ProgramMemory::getTokValue(nonneg int exprid, const Token*& result) const
bool ProgramMemory::getTokValue(nonneg int exprid, const Token*& result, bool impossible) const
{
const ValueFlow::Value* value = getValue(exprid);
const ValueFlow::Value* value = getValue(exprid, impossible);
if (value && value->isTokValue()) {
result = value->tokvalue;
return true;
Expand All@@ -125,18 +140,18 @@ bool ProgramMemory::getTokValue(nonneg int exprid, const Token*& result) const
}

// cppcheck-suppress unusedFunction
bool ProgramMemory::getContainerSizeValue(nonneg int exprid, MathLib::bigint& result) const
bool ProgramMemory::getContainerSizeValue(nonneg int exprid, MathLib::bigint& result, bool impossible) const
{
const ValueFlow::Value* value = getValue(exprid);
const ValueFlow::Value* value = getValue(exprid, impossible);
if (value && value->isContainerSizeValue()) {
result = value->intvalue;
return true;
}
return false;
}
bool ProgramMemory::getContainerEmptyValue(nonneg int exprid, MathLib::bigint& result) const
bool ProgramMemory::getContainerEmptyValue(nonneg int exprid, MathLib::bigint& result, bool impossible) const
{
const ValueFlow::Value* value = getValue(exprid, true);
const ValueFlow::Value* value = getValue(exprid, impossible);
if (value && value->isContainerSizeValue()) {
if (value->isImpossible() && value->intvalue == 0) {
result = false;
Expand DownExpand Up@@ -165,28 +180,10 @@ void ProgramMemory::setUnknown(const Token* expr) {
(*mValues)[expr].valueType = ValueFlow::Value::ValueType::UNINIT;
}

bool ProgramMemory::hasValue(nonneg int exprid) const
bool ProgramMemory::hasValue(nonneg int exprid, bool impossible) const
{
const auto it = find(exprid);
return it != mValues->cend();
}

const ValueFlow::Value& ProgramMemory::at(nonneg int exprid) const {
const auto it = find(exprid);
if (it == mValues->cend()) {
throw std::out_of_range("ProgramMemory::at");
}
return it->second;
}

ValueFlow::Value& ProgramMemory::at(nonneg int exprid) {
copyOnWrite();

const auto it = find(exprid);
if (it == mValues->end()) {
throw std::out_of_range("ProgramMemory::at");
}
return it->second;
return it != mValues->cend() && (impossible || !it->second.isImpossible());
}

void ProgramMemory::erase_if(const std::function<bool(const ExprIdToken&)>& pred)
Expand DownExpand Up@@ -257,6 +254,7 @@ ProgramMemory::Map::const_iterator ProgramMemory::find(nonneg int exprid) const
return cvalues.find(ExprIdToken::create(exprid));
}

// need to call copyOnWrite() before calling this
ProgramMemory::Map::iterator ProgramMemory::find(nonneg int exprid)
{
return mValues->find(ExprIdToken::create(exprid));
Expand DownExpand Up@@ -1366,10 +1364,9 @@ namespace {

ValueFlow::Value executeMultiCondition(bool b, const Token* expr)
{
if (pm->hasValue(expr->exprId())) {
const ValueFlow::Value& v = utils::as_const(*pm).at(expr->exprId());
if (v.isIntValue())
return v;
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId(), true)) {

Check warning

Code scanning / Cppcheck Premium

Do not dereference null pointers Warning

Do not dereference null pointers

@danmardanmarJan 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you think some cppcheck premium warnings are too noisy then please feel free to complain. I don't want that cppcheck premium is in the way for the team.

if (v->isIntValue())
return *v;
}

// Evaluate recursively if there are no exprids
Expand DownExpand Up@@ -1498,18 +1495,18 @@ namespace {
if (rhs.isUninitValue())
return unknown();
if (expr->str() != "=") {
if (!pm->hasValue(expr->astOperand1()->exprId()))
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId(), true);
if (!lhs)
return unknown();
ValueFlow::Value& lhs = pm->at(expr->astOperand1()->exprId());
rhs = evaluate(removeAssign(expr->str()), lhs, rhs);
if (lhs.isIntValue())
ValueFlow::Value::visitValue(rhs, std::bind(assign{}, std::ref(lhs.intvalue), std::placeholders::_1));
else if (lhs.isFloatValue())
rhs = evaluate(removeAssign(expr->str()), *lhs, rhs);
if (lhs->isIntValue())
ValueFlow::Value::visitValue(rhs, std::bind(assign{}, std::ref(lhs->intvalue), std::placeholders::_1));
else if (lhs->isFloatValue())
ValueFlow::Value::visitValue(rhs,
std::bind(assign{}, std::ref(lhs.floatValue), std::placeholders::_1));
std::bind(assign{}, std::ref(lhs->floatValue), std::placeholders::_1));
else
return unknown();
return lhs;
return *lhs;
}
pm->setValue(expr->astOperand1(), rhs);
return rhs;
Expand All@@ -1521,20 +1518,20 @@ namespace {
execute(expr->astOperand1());
return execute(expr->astOperand2());
} else if (expr->tokType() == Token::eIncDecOp && expr->astOperand1() && expr->astOperand1()->exprId() != 0) {
if (!pm->hasValue(expr->astOperand1()->exprId()))
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId(), true);
if (!lhs)
return ValueFlow::Value::unknown();
ValueFlow::Value& lhs = pm->at(expr->astOperand1()->exprId());
if (!lhs.isIntValue())
if (!lhs->isIntValue())
return unknown();
// overflow
if (!lhs.isImpossible() && lhs.intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
if (!lhs->isImpossible() && lhs->intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
return unknown();

if (expr->str() == "++")
lhs.intvalue++;
lhs->intvalue++;
else
lhs.intvalue--;
return lhs;
lhs->intvalue--;
return *lhs;
} else if (expr->str() == "[" && expr->astOperand1() && expr->astOperand2()) {
const Token* tokvalue = nullptr;
if (!pm->getTokValue(expr->astOperand1()->exprId(), tokvalue)) {
Expand DownExpand Up@@ -1625,13 +1622,16 @@ namespace {
}
return execute(expr->astOperand1());
}
if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) {
ValueFlow::Value result = utils::as_const(*pm).at(expr->exprId());
if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
result.intvalue = !result.intvalue;
result.setKnown();
if (expr->exprId() > 0) {
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId(), true))
{
ValueFlow::Value result = *v;
if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
result.intvalue = !result.intvalue;
result.setKnown();
}
return result;
}
return result;
}

if (Token::Match(expr->previous(), ">|%name% {|(")) {
Expand DownExpand Up@@ -1681,14 +1681,16 @@ namespace {
}
// Check if function modifies argument
visitAstNodes(expr->astOperand2(), [&](const Token* child) {
if (child->exprId() > 0 && pm->hasValue(child->exprId())) {
ValueFlow::Value& v = pm->at(child->exprId());
if (v.valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE) {
if (ValueFlow::isContainerSizeChanged(child, v.indirect, settings))
v = unknown();
} else if (v.valueType != ValueFlow::Value::ValueType::UNINIT) {
if (isVariableChanged(child, v.indirect, settings))
v = unknown();
if (child->exprId() > 0) {
if (ValueFlow::Value* v = pm->getValue(child->exprId(), true))
{
if (v->valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE) {
if (ValueFlow::isContainerSizeChanged(child, v->indirect, settings))
*v = unknown();
} else if (v->valueType != ValueFlow::Value::ValueType::UNINIT) {
if (isVariableChanged(child, v->indirect, settings))
*v = unknown();
}
}
}
return ChildrenToVisit::op1_and_op2;
Expand DownExpand Up@@ -1740,22 +1742,27 @@ namespace {
return v;
if (!expr)
return v;
if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) {
if (updateValue(v, utils::as_const(*pm).at(expr->exprId())))
return v;
if (expr->exprId() > 0) {
if (const ValueFlow::Value* val = utils::as_const(*pm).getValue(expr->exprId(), true))
{
if (updateValue(v, *val))
return v;
}
}
// Find symbolic values
for (const ValueFlow::Value& value : expr->values()) {
if (!value.isSymbolicValue())
continue;
if (!value.isKnown())
continue;
if (value.tokvalue->exprId() > 0 && !pm->hasValue(value.tokvalue->exprId()))
if (value.tokvalue->exprId() == 0)
continue;
Comment on lines -1753 to +1759

@firewavefirewaveAug 25, 2025•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been separate before since an at() call with no exprid would have failed.

const ValueFlow::Value* v_p = utils::as_const(*pm).getValue(value.tokvalue->exprId(), true);
if (!v_p)
continue;
const ValueFlow::Value& v_ref = utils::as_const(*pm).at(value.tokvalue->exprId());
if (!v_ref.isIntValue() && value.intvalue != 0)
if (!v_p->isIntValue() && value.intvalue != 0)
continue;
ValueFlow::Value v2 = v_ref;
ValueFlow::Value v2 = *v_p;
v2.intvalue += value.intvalue;
return v2;
}
Expand Down
14 changes: 6 additions & 8 deletions lib/programmemory.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -110,21 +110,19 @@ struct CPPCHECKLIB ProgramMemory {

void setValue(const Token* expr, const ValueFlow::Value& value);
const ValueFlow::Value* getValue(nonneg int exprid, bool impossible = false) const;
ValueFlow::Value* getValue(nonneg int exprid, bool impossible = false);

bool getIntValue(nonneg int exprid, MathLib::bigint& result) const;
bool getIntValue(nonneg int exprid, MathLib::bigint& result, bool impossible = false) const;
void setIntValue(const Token* expr, MathLib::bigint value, bool impossible = false);

bool getContainerSizeValue(nonneg int exprid, MathLib::bigint& result) const;
bool getContainerEmptyValue(nonneg int exprid, MathLib::bigint& result) const;
bool getContainerSizeValue(nonneg int exprid, MathLib::bigint& result, bool impossible = false) const;
bool getContainerEmptyValue(nonneg int exprid, MathLib::bigint& result, bool impossible = false) const;

@firewavefirewaveSep 15, 2025•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I flipped the default behavior on getContainerEmptyValue() but addressed it via an explicit parameter in its only usage in vf_analyzers.cpp.

@pfultz2pfultz2Sep 16, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont add the impossible parameters. They dont make sense at all.

@firewavefirewaveSep 16, 2025

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read my comment. It was to make it clear what they are doing as they were inconsistent. But if we move the impossible logic out of them they will go away.

void setContainerSizeValue(const Token* expr, MathLib::bigint value, bool isEqual = true);

void setUnknown(const Token* expr);

bool getTokValue(nonneg int exprid, const Token*& result) const;
bool hasValue(nonneg int exprid) const;

const ValueFlow::Value& at(nonneg int exprid) const;
ValueFlow::Value& at(nonneg int exprid);
bool getTokValue(nonneg int exprid, const Token*& result, bool impossible = false) const;
bool hasValue(nonneg int exprid, bool impossible = true) const;

void erase_if(const std::function<bool(const ExprIdToken&)>& pred);

Expand Down
3 changes: 2 additions & 1 deletion lib/vf_analyzers.cpp
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -716,7 +716,8 @@ struct ValueFlowAnalyzer : Analyzer {
return {value->intvalue == 0};
ProgramMemory pm = pms.get(tok, ctx, getProgramState());
MathLib::bigint out = 0;
if (pm.getContainerEmptyValue(tok->exprId(), out))
// TODO: do we really went to return an impossible value?
if (pm.getContainerEmptyValue(tok->exprId(), out, true))

@pfultz2pfultz2Sep 16, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the correct behaviour, but the parameter and the comment makes it look like its wrong. The getContainerEmptyValue returns true or false if the container is empty or not. It will use impossible values internally to determine this as well(its really an implementation detail though).

This is why the impossible parameter should not be added.

@firewavefirewaveSep 16, 2025

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is inconsistent with the other getters. But if we move the impossible logic out of those of them as suggested, things will be clear.

@firewavefirewaveFeb 18, 2026

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in another comment the impossible handling cannot easily be moved out of these functions.

return {static_cast<int>(out)};
return {};
}
Expand Down
21 changes: 7 additions & 14 deletions test/testprogrammemory.cpp
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -35,7 +35,6 @@ class TestProgramMemory : public TestFixture {
TEST_CASE(copyOnWrite);
TEST_CASE(hasValue);
TEST_CASE(getValue);
TEST_CASE(at);
}

void copyOnWrite() const {
Expand All@@ -45,19 +44,19 @@ class TestProgramMemory : public TestFixture {
tok->exprId(id);

ProgramMemory pm;
const ValueFlow::Value* v = pm.getValue(id);
const ValueFlow::Value* v = utils::as_const(pm).getValue(id, false);
ASSERT(!v);
pm.setValue(tok, ValueFlow::Value{41});

v = pm.getValue(id);
v = utils::as_const(pm).getValue(id, false);
ASSERT(v);
ASSERT_EQUALS(41, v->intvalue);

// create a copy
ProgramMemory pm2 = pm;

// make sure the value was copied
v = pm2.getValue(id);
v = utils::as_const(pm2).getValue(id, false);
ASSERT(v);
ASSERT_EQUALS(41, v->intvalue);

Expand All@@ -71,17 +70,17 @@ class TestProgramMemory : public TestFixture {
pm3.setValue(tok, ValueFlow::Value{43});

// make sure the value was set
v = pm2.getValue(id);
v = utils::as_const(pm2).getValue(id, false);
ASSERT(v);
ASSERT_EQUALS(42, v->intvalue);

// make sure the value was set
v = pm3.getValue(id);
v = utils::as_const(pm3).getValue(id, false);
ASSERT(v);
ASSERT_EQUALS(43, v->intvalue);

// make sure the original value remains unchanged
v = pm.getValue(id);
v = utils::as_const(pm).getValue(id, false);
ASSERT(v);
ASSERT_EQUALS(41, v->intvalue);
}
Expand All@@ -93,13 +92,7 @@ class TestProgramMemory : public TestFixture {

void getValue() const {
ProgramMemory pm;
ASSERT(!pm.getValue(123));
}

void at() const {
ProgramMemory pm;
ASSERT_THROW_EQUALS(pm.at(123), std::out_of_range, "ProgramMemory::at");
ASSERT_THROW_EQUALS(utils::as_const(pm).at(123), std::out_of_range, "ProgramMemory::at");
ASSERT(!utils::as_const(pm).getValue(123, false));
}
};

Expand Down
Loading