Skip to content

Commit 1cb862f

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

File tree

3 files changed

+63
-67
lines changed

3 files changed

+63
-67
lines changed

‎lib/programmemory.cpp‎

Lines changed: 62 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,18 @@ const ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossib
8585
return nullptr;
8686
}
8787

88+
ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid)
89+
{
90+
// TODO: avoid copy if no value is found
91+
copyOnWrite();
92+
93+
const auto it = find(exprid);
94+
const bool found = it != mValues->cend();
95+
if (found)
96+
return &it->second;
97+
return nullptr;
98+
}
99+
88100
// cppcheck-suppress unusedFunction
89101
bool ProgramMemory::getIntValue(nonneg int exprid, MathLib::bigint& result) const
90102
{
@@ -161,24 +173,6 @@ bool ProgramMemory::hasValue(nonneg int exprid) const
161173
return it != mValues->cend();
162174
}
163175

164-
const ValueFlow::Value& ProgramMemory::at(nonneg int exprid) const {
165-
const auto it = find(exprid);
166-
if (it == mValues->cend()) {
167-
throw std::out_of_range("ProgramMemory::at");
168-
}
169-
return it->second;
170-
}
171-
172-
ValueFlow::Value& ProgramMemory::at(nonneg int exprid) {
173-
copyOnWrite();
174-
175-
const auto it = find(exprid);
176-
if (it == mValues->end()) {
177-
throw std::out_of_range("ProgramMemory::at");
178-
}
179-
return it->second;
180-
}
181-
182176
void ProgramMemory::erase_if(const std::function<bool(const ExprIdToken&)>& pred)
183177
{
184178
if (mValues->empty())
@@ -244,6 +238,7 @@ ProgramMemory::Map::const_iterator ProgramMemory::find(nonneg int exprid) const
244238
});
245239
}
246240

241+
// need to call copyOnWrite() before calling this
247242
ProgramMemory::Map::iterator ProgramMemory::find(nonneg int exprid)
248243
{
249244
return std::find_if(mValues->begin(), mValues->end(), [&exprid](const Map::value_type& entry) {
@@ -1343,10 +1338,9 @@ namespace {
13431338

13441339
ValueFlow::Value executeMultiCondition(bool b, const Token* expr)
13451340
{
1346-
if (pm->hasValue(expr->exprId())) {
1347-
const ValueFlow::Value& v = utils::as_const(*pm).at(expr->exprId());
1348-
if (v.isIntValue())
1349-
return v;
1341+
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId())) {
1342+
if (v->isIntValue())
1343+
return *v;
13501344
}
13511345

13521346
// Evaluate recursively if there are no exprids
@@ -1475,18 +1469,18 @@ namespace {
14751469
if (rhs.isUninitValue())
14761470
return unknown();
14771471
if (expr->str() != "=") {
1478-
if (!pm->hasValue(expr->astOperand1()->exprId()))
1472+
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId());
1473+
if (!lhs)
14791474
return unknown();
1480-
ValueFlow::Value& lhs = pm->at(expr->astOperand1()->exprId());
1481-
rhs = evaluate(removeAssign(expr->str()), lhs, rhs);
1482-
if (lhs.isIntValue())
1483-
ValueFlow::Value::visitValue(rhs, std::bind(assign{}, std::ref(lhs.intvalue), std::placeholders::_1));
1484-
else if (lhs.isFloatValue())
1475+
rhs = evaluate(removeAssign(expr->str()), *lhs, rhs);
1476+
if (lhs->isIntValue())
1477+
ValueFlow::Value::visitValue(rhs, std::bind(assign{}, std::ref(lhs->intvalue), std::placeholders::_1));
1478+
else if (lhs->isFloatValue())
14851479
ValueFlow::Value::visitValue(rhs,
1486-
std::bind(assign{}, std::ref(lhs.floatValue), std::placeholders::_1));
1480+
std::bind(assign{}, std::ref(lhs->floatValue), std::placeholders::_1));
14871481
else
14881482
return unknown();
1489-
return lhs;
1483+
return *lhs;
14901484
}
14911485
pm->setValue(expr->astOperand1(), rhs);
14921486
return rhs;
@@ -1498,20 +1492,20 @@ namespace {
14981492
execute(expr->astOperand1());
14991493
return execute(expr->astOperand2());
15001494
} else if (expr->tokType() == Token::eIncDecOp && expr->astOperand1() && expr->astOperand1()->exprId() != 0) {
1501-
if (!pm->hasValue(expr->astOperand1()->exprId()))
1495+
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId());
1496+
if (!lhs)
15021497
return ValueFlow::Value::unknown();
1503-
ValueFlow::Value& lhs = pm->at(expr->astOperand1()->exprId());
1504-
if (!lhs.isIntValue())
1498+
if (!lhs->isIntValue())
15051499
return unknown();
15061500
// overflow
1507-
if (!lhs.isImpossible() && lhs.intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
1501+
if (!lhs->isImpossible() && lhs->intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
15081502
return unknown();
15091503

15101504
if (expr->str() == "++")
1511-
lhs.intvalue++;
1505+
lhs->intvalue++;
15121506
else
1513-
lhs.intvalue--;
1514-
return lhs;
1507+
lhs->intvalue--;
1508+
return *lhs;
15151509
} else if (expr->str() == "[" && expr->astOperand1() && expr->astOperand2()) {
15161510
const Token* tokvalue = nullptr;
15171511
if (!pm->getTokValue(expr->astOperand1()->exprId(), tokvalue)) {
@@ -1602,13 +1596,16 @@ namespace {
16021596
}
16031597
return execute(expr->astOperand1());
16041598
}
1605-
if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) {
1606-
ValueFlow::Value result = utils::as_const(*pm).at(expr->exprId());
1607-
if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
1608-
result.intvalue = !result.intvalue;
1609-
result.setKnown();
1599+
if (expr->exprId() > 0) {
1600+
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId()))
1601+
{
1602+
ValueFlow::Value result = *v;
1603+
if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
1604+
result.intvalue = !result.intvalue;
1605+
result.setKnown();
1606+
}
1607+
return result;
16101608
}
1611-
return result;
16121609
}
16131610

16141611
if (Token::Match(expr->previous(), ">|%name% {|(")) {
@@ -1658,14 +1655,16 @@ namespace {
16581655
}
16591656
// Check if function modifies argument
16601657
visitAstNodes(expr->astOperand2(), [&](const Token* child) {
1661-
if (child->exprId() > 0 && pm->hasValue(child->exprId())) {
1662-
ValueFlow::Value& v = pm->at(child->exprId());
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();
1658+
if (child->exprId() > 0) {
1659+
if (ValueFlow::Value* v = pm->getValue(child->exprId()))
1660+
{
1661+
if (v->valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE) {
1662+
if (ValueFlow::isContainerSizeChanged(child, v->indirect, settings))
1663+
*v = unknown();
1664+
} else if (v->valueType != ValueFlow::Value::ValueType::UNINIT) {
1665+
if (isVariableChanged(child, v->indirect, settings))
1666+
*v = unknown();
1667+
}
16691668
}
16701669
}
16711670
return ChildrenToVisit::op1_and_op2;
@@ -1717,22 +1716,27 @@ namespace {
17171716
return v;
17181717
if (!expr)
17191718
return v;
1720-
if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) {
1721-
if (updateValue(v, utils::as_const(*pm).at(expr->exprId())))
1722-
return v;
1719+
if (expr->exprId() > 0) {
1720+
if (const ValueFlow::Value* val = utils::as_const(*pm).getValue(expr->exprId()))
1721+
{
1722+
if (updateValue(v, *val))
1723+
return v;
1724+
}
17231725
}
17241726
// Find symbolic values
17251727
for (const ValueFlow::Value& value : expr->values()) {
17261728
if (!value.isSymbolicValue())
17271729
continue;
17281730
if (!value.isKnown())
17291731
continue;
1730-
if (value.tokvalue->exprId() > 0 && !pm->hasValue(value.tokvalue->exprId()))
1732+
if (value.tokvalue->exprId() == 0)
1733+
continue;
1734+
const ValueFlow::Value* v_p = utils::as_const(*pm).getValue(value.tokvalue->exprId());
1735+
if (!v_p)
17311736
continue;
1732-
const ValueFlow::Value& v_ref = utils::as_const(*pm).at(value.tokvalue->exprId());
1733-
if (!v_ref.isIntValue() && value.intvalue != 0)
1737+
if (!v_p->isIntValue() && value.intvalue != 0)
17341738
continue;
1735-
ValueFlow::Value v2 = v_ref;
1739+
ValueFlow::Value v2 = *v_p;
17361740
v2.intvalue += value.intvalue;
17371741
return v2;
17381742
}

‎lib/programmemory.h‎

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

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

105106
bool getIntValue(nonneg int exprid, MathLib::bigint& result) const;
106107
void setIntValue(const Token* expr, MathLib::bigint value, bool impossible = false);
@@ -114,9 +115,6 @@ struct CPPCHECKLIB ProgramMemory {
114115
bool getTokValue(nonneg int exprid, const Token*& result) const;
115116
bool hasValue(nonneg int exprid) const;
116117

117-
const ValueFlow::Value& at(nonneg int exprid) const;
118-
ValueFlow::Value& at(nonneg int exprid);
119-
120118
void erase_if(const std::function<bool(const ExprIdToken&)>& pred);
121119

122120
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)