Skip to content

Commit f84c57f

Browse files
firewavefirewave
committed
ProgramMemory: avoid duplicated lookups / removed at() [skip ci]
1 parent d9a054c commit f84c57f

File tree

3 files changed

+66
-67
lines changed

3 files changed

+66
-67
lines changed

‎lib/programmemory.cpp‎

Lines changed: 65 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,21 @@ const ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossib
8888
return nullptr;
8989
}
9090

91+
ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid)
92+
{
93+
if (mValues->empty())
94+
return nullptr;
95+
96+
// TODO: avoid copy if no value is found
97+
copyOnWrite();
98+
99+
const auto it = find(exprid);
100+
const bool found = it != mValues->cend();
101+
if (found)
102+
return &it->second;
103+
return nullptr;
104+
}
105+
91106
// cppcheck-suppress unusedFunction
92107
bool ProgramMemory::getIntValue(nonneg int exprid, MathLib::bigint& result) const
93108
{
@@ -164,24 +179,6 @@ bool ProgramMemory::hasValue(nonneg int exprid) const
164179
return it != mValues->cend();
165180
}
166181

167-
const ValueFlow::Value& ProgramMemory::at(nonneg int exprid) const {
168-
const auto it = find(exprid);
169-
if (it == mValues->cend()) {
170-
throw std::out_of_range("ProgramMemory::at");
171-
}
172-
return it->second;
173-
}
174-
175-
ValueFlow::Value& ProgramMemory::at(nonneg int exprid) {
176-
copyOnWrite();
177-
178-
const auto it = find(exprid);
179-
if (it == mValues->end()) {
180-
throw std::out_of_range("ProgramMemory::at");
181-
}
182-
return it->second;
183-
}
184-
185182
void ProgramMemory::erase_if(const std::function<bool(const ExprIdToken&)>& pred)
186183
{
187184
if (mValues->empty())
@@ -245,6 +242,7 @@ ProgramMemory::Map::const_iterator ProgramMemory::find(nonneg int exprid) const
245242
return cvalues.find(ExprIdToken::create(exprid));
246243
}
247244

245+
// need to call copyOnWrite() before calling this
248246
ProgramMemory::Map::iterator ProgramMemory::find(nonneg int exprid)
249247
{
250248
return mValues->find(ExprIdToken::create(exprid));
@@ -1342,10 +1340,9 @@ namespace {
13421340

13431341
ValueFlow::Value executeMultiCondition(bool b, const Token* expr)
13441342
{
1345-
if (pm->hasValue(expr->exprId())) {
1346-
const ValueFlow::Value& v = utils::as_const(*pm).at(expr->exprId());
1347-
if (v.isIntValue())
1348-
return v;
1343+
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId())) {
1344+
if (v->isIntValue())
1345+
return *v;
13491346
}
13501347

13511348
// Evaluate recursively if there are no exprids
@@ -1474,18 +1471,18 @@ namespace {
14741471
if (rhs.isUninitValue())
14751472
return unknown();
14761473
if (expr->str() != "=") {
1477-
if (!pm->hasValue(expr->astOperand1()->exprId()))
1474+
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId());
1475+
if (!lhs)
14781476
return unknown();
1479-
ValueFlow::Value& lhs = pm->at(expr->astOperand1()->exprId());
1480-
rhs = evaluate(removeAssign(expr->str()), lhs, rhs);
1481-
if (lhs.isIntValue())
1482-
ValueFlow::Value::visitValue(rhs, std::bind(assign{}, std::ref(lhs.intvalue), std::placeholders::_1));
1483-
else if (lhs.isFloatValue())
1477+
rhs = evaluate(removeAssign(expr->str()), *lhs, rhs);
1478+
if (lhs->isIntValue())
1479+
ValueFlow::Value::visitValue(rhs, std::bind(assign{}, std::ref(lhs->intvalue), std::placeholders::_1));
1480+
else if (lhs->isFloatValue())
14841481
ValueFlow::Value::visitValue(rhs,
1485-
std::bind(assign{}, std::ref(lhs.floatValue), std::placeholders::_1));
1482+
std::bind(assign{}, std::ref(lhs->floatValue), std::placeholders::_1));
14861483
else
14871484
return unknown();
1488-
return lhs;
1485+
return *lhs;
14891486
}
14901487
pm->setValue(expr->astOperand1(), rhs);
14911488
return rhs;
@@ -1497,20 +1494,20 @@ namespace {
14971494
execute(expr->astOperand1());
14981495
return execute(expr->astOperand2());
14991496
} else if (expr->tokType() == Token::eIncDecOp && expr->astOperand1() && expr->astOperand1()->exprId() != 0) {
1500-
if (!pm->hasValue(expr->astOperand1()->exprId()))
1497+
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId());
1498+
if (!lhs)
15011499
return ValueFlow::Value::unknown();
1502-
ValueFlow::Value& lhs = pm->at(expr->astOperand1()->exprId());
1503-
if (!lhs.isIntValue())
1500+
if (!lhs->isIntValue())
15041501
return unknown();
15051502
// overflow
1506-
if (!lhs.isImpossible() && lhs.intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
1503+
if (!lhs->isImpossible() && lhs->intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
15071504
return unknown();
15081505

15091506
if (expr->str() == "++")
1510-
lhs.intvalue++;
1507+
lhs->intvalue++;
15111508
else
1512-
lhs.intvalue--;
1513-
return lhs;
1509+
lhs->intvalue--;
1510+
return *lhs;
15141511
} else if (expr->str() == "[" && expr->astOperand1() && expr->astOperand2()) {
15151512
const Token* tokvalue = nullptr;
15161513
if (!pm->getTokValue(expr->astOperand1()->exprId(), tokvalue)) {
@@ -1601,13 +1598,16 @@ namespace {
16011598
}
16021599
return execute(expr->astOperand1());
16031600
}
1604-
if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) {
1605-
ValueFlow::Value result = utils::as_const(*pm).at(expr->exprId());
1606-
if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
1607-
result.intvalue = !result.intvalue;
1608-
result.setKnown();
1601+
if (expr->exprId() > 0) {
1602+
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId()))
1603+
{
1604+
ValueFlow::Value result = *v;
1605+
if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
1606+
result.intvalue = !result.intvalue;
1607+
result.setKnown();
1608+
}
1609+
return result;
16091610
}
1610-
return result;
16111611
}
16121612

16131613
if (Token::Match(expr->previous(), ">|%name% {|(")) {
@@ -1657,14 +1657,16 @@ namespace {
16571657
}
16581658
// Check if function modifies argument
16591659
visitAstNodes(expr->astOperand2(), [&](const Token* child) {
1660-
if (child->exprId() > 0 && pm->hasValue(child->exprId())) {
1661-
ValueFlow::Value& v = pm->at(child->exprId());
1662-
if (v.valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE) {
1663-
if (ValueFlow::isContainerSizeChanged(child, v.indirect, settings))
1664-
v = unknown();
1665-
} else if (v.valueType != ValueFlow::Value::ValueType::UNINIT) {
1666-
if (isVariableChanged(child, v.indirect, settings))
1667-
v = unknown();
1660+
if (child->exprId() > 0) {
1661+
if (ValueFlow::Value* v = pm->getValue(child->exprId()))
1662+
{
1663+
if (v->valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE) {
1664+
if (ValueFlow::isContainerSizeChanged(child, v->indirect, settings))
1665+
*v = unknown();
1666+
} else if (v->valueType != ValueFlow::Value::ValueType::UNINIT) {
1667+
if (isVariableChanged(child, v->indirect, settings))
1668+
*v = unknown();
1669+
}
16681670
}
16691671
}
16701672
return ChildrenToVisit::op1_and_op2;
@@ -1716,22 +1718,27 @@ namespace {
17161718
return v;
17171719
if (!expr)
17181720
return v;
1719-
if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) {
1720-
if (updateValue(v, utils::as_const(*pm).at(expr->exprId())))
1721-
return v;
1721+
if (expr->exprId() > 0) {
1722+
if (const ValueFlow::Value* val = utils::as_const(*pm).getValue(expr->exprId()))
1723+
{
1724+
if (updateValue(v, *val))
1725+
return v;
1726+
}
17221727
}
17231728
// Find symbolic values
17241729
for (const ValueFlow::Value& value : expr->values()) {
17251730
if (!value.isSymbolicValue())
17261731
continue;
17271732
if (!value.isKnown())
17281733
continue;
1729-
if (value.tokvalue->exprId() > 0 && !pm->hasValue(value.tokvalue->exprId()))
1734+
if (value.tokvalue->exprId() == 0)
1735+
continue;
1736+
const ValueFlow::Value* v_p = utils::as_const(*pm).getValue(value.tokvalue->exprId());
1737+
if (!v_p)
17301738
continue;
1731-
const ValueFlow::Value& v_ref = utils::as_const(*pm).at(value.tokvalue->exprId());
1732-
if (!v_ref.isIntValue() && value.intvalue != 0)
1739+
if (!v_p->isIntValue() && value.intvalue != 0)
17331740
continue;
1734-
ValueFlow::Value v2 = v_ref;
1741+
ValueFlow::Value v2 = *v_p;
17351742
v2.intvalue += value.intvalue;
17361743
return v2;
17371744
}

‎lib/programmemory.h‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ struct CPPCHECKLIB ProgramMemory {
110110

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

114115
bool getIntValue(nonneg int exprid, MathLib::bigint& result) const;
115116
void setIntValue(const Token* expr, MathLib::bigint value, bool impossible = false);
@@ -123,9 +124,6 @@ struct CPPCHECKLIB ProgramMemory {
123124
bool getTokValue(nonneg int exprid, const Token*& result) const;
124125
bool hasValue(nonneg int exprid) const;
125126

126-
const ValueFlow::Value& at(nonneg int exprid) const;
127-
ValueFlow::Value& at(nonneg int exprid);
128-
129127
void erase_if(const std::function<bool(const ExprIdToken&)>& pred);
130128

131129
void swap(ProgramMemory &pm) NOEXCEPT;

‎test/testprogrammemory.cpp‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,12 +92,6 @@ class TestProgramMemory : public TestFixture {
9292
ProgramMemory pm;
9393
ASSERT(!pm.getValue(123));
9494
}
95-
96-
void at() const {
97-
ProgramMemory pm;
98-
ASSERT_THROW_EQUALS_2(pm.at(123), std::out_of_range, "ProgramMemory::at");
99-
ASSERT_THROW_EQUALS_2(utils::as_const(pm).at(123), std::out_of_range, "ProgramMemory::at");
100-
}
10195
};
10296

10397
REGISTER_TEST(TestProgramMemory)

0 commit comments

Comments
 (0)