Skip to content

Commit 7a04197

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

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
@@ -85,6 +85,21 @@ 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+
if (mValues->empty())
91+
return nullptr;
92+
93+
// TODO: avoid copy if no value is found
94+
copyOnWrite();
95+
96+
const auto it = find(exprid);
97+
const bool found = it != mValues->cend();
98+
if (found)
99+
return &it->second;
100+
return nullptr;
101+
}
102+
88103
// cppcheck-suppress unusedFunction
89104
bool ProgramMemory::getIntValue(nonneg int exprid, MathLib::bigint& result) const
90105
{
@@ -161,24 +176,6 @@ bool ProgramMemory::hasValue(nonneg int exprid) const
161176
return it != mValues->cend();
162177
}
163178

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-
182179
void ProgramMemory::erase_if(const std::function<bool(const ExprIdToken&)>& pred)
183180
{
184181
if (mValues->empty())
@@ -244,6 +241,7 @@ ProgramMemory::Map::const_iterator ProgramMemory::find(nonneg int exprid) const
244241
});
245242
}
246243

244+
// need to call copyOnWrite() before calling this
247245
ProgramMemory::Map::iterator ProgramMemory::find(nonneg int exprid)
248246
{
249247
return std::find_if(mValues->begin(), mValues->end(), [&exprid](const Map::value_type& entry) {
@@ -1343,10 +1341,9 @@ namespace {
13431341

13441342
ValueFlow::Value executeMultiCondition(bool b, const Token* expr)
13451343
{
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;
1344+
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId())) {
1345+
if (v->isIntValue())
1346+
return *v;
13501347
}
13511348

13521349
// Evaluate recursively if there are no exprids
@@ -1475,18 +1472,18 @@ namespace {
14751472
if (rhs.isUninitValue())
14761473
return unknown();
14771474
if (expr->str() != "=") {
1478-
if (!pm->hasValue(expr->astOperand1()->exprId()))
1475+
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId());
1476+
if (!lhs)
14791477
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())
1478+
rhs = evaluate(removeAssign(expr->str()), *lhs, rhs);
1479+
if (lhs->isIntValue())
1480+
ValueFlow::Value::visitValue(rhs, std::bind(assign{}, std::ref(lhs->intvalue), std::placeholders::_1));
1481+
else if (lhs->isFloatValue())
14851482
ValueFlow::Value::visitValue(rhs,
1486-
std::bind(assign{}, std::ref(lhs.floatValue), std::placeholders::_1));
1483+
std::bind(assign{}, std::ref(lhs->floatValue), std::placeholders::_1));
14871484
else
14881485
return unknown();
1489-
return lhs;
1486+
return *lhs;
14901487
}
14911488
pm->setValue(expr->astOperand1(), rhs);
14921489
return rhs;
@@ -1498,20 +1495,20 @@ namespace {
14981495
execute(expr->astOperand1());
14991496
return execute(expr->astOperand2());
15001497
} else if (expr->tokType() == Token::eIncDecOp && expr->astOperand1() && expr->astOperand1()->exprId() != 0) {
1501-
if (!pm->hasValue(expr->astOperand1()->exprId()))
1498+
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId());
1499+
if (!lhs)
15021500
return ValueFlow::Value::unknown();
1503-
ValueFlow::Value& lhs = pm->at(expr->astOperand1()->exprId());
1504-
if (!lhs.isIntValue())
1501+
if (!lhs->isIntValue())
15051502
return unknown();
15061503
// overflow
1507-
if (!lhs.isImpossible() && lhs.intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
1504+
if (!lhs->isImpossible() && lhs->intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
15081505
return unknown();
15091506

15101507
if (expr->str() == "++")
1511-
lhs.intvalue++;
1508+
lhs->intvalue++;
15121509
else
1513-
lhs.intvalue--;
1514-
return lhs;
1510+
lhs->intvalue--;
1511+
return *lhs;
15151512
} else if (expr->str() == "[" && expr->astOperand1() && expr->astOperand2()) {
15161513
const Token* tokvalue = nullptr;
15171514
if (!pm->getTokValue(expr->astOperand1()->exprId(), tokvalue)) {
@@ -1602,13 +1599,16 @@ namespace {
16021599
}
16031600
return execute(expr->astOperand1());
16041601
}
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();
1602+
if (expr->exprId() > 0) {
1603+
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId()))
1604+
{
1605+
ValueFlow::Value result = *v;
1606+
if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
1607+
result.intvalue = !result.intvalue;
1608+
result.setKnown();
1609+
}
1610+
return result;
16101611
}
1611-
return result;
16121612
}
16131613

16141614
if (Token::Match(expr->previous(), ">|%name% {|(")) {
@@ -1658,14 +1658,16 @@ namespace {
16581658
}
16591659
// Check if function modifies argument
16601660
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();
1661+
if (child->exprId() > 0) {
1662+
if (ValueFlow::Value* v = pm->getValue(child->exprId()))
1663+
{
1664+
if (v->valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE) {
1665+
if (ValueFlow::isContainerSizeChanged(child, v->indirect, settings))
1666+
*v = unknown();
1667+
} else if (v->valueType != ValueFlow::Value::ValueType::UNINIT) {
1668+
if (isVariableChanged(child, v->indirect, settings))
1669+
*v = unknown();
1670+
}
16691671
}
16701672
}
16711673
return ChildrenToVisit::op1_and_op2;
@@ -1717,22 +1719,27 @@ namespace {
17171719
return v;
17181720
if (!expr)
17191721
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;
1722+
if (expr->exprId() > 0) {
1723+
if (const ValueFlow::Value* val = utils::as_const(*pm).getValue(expr->exprId()))
1724+
{
1725+
if (updateValue(v, *val))
1726+
return v;
1727+
}
17231728
}
17241729
// Find symbolic values
17251730
for (const ValueFlow::Value& value : expr->values()) {
17261731
if (!value.isSymbolicValue())
17271732
continue;
17281733
if (!value.isKnown())
17291734
continue;
1730-
if (value.tokvalue->exprId() > 0 && !pm->hasValue(value.tokvalue->exprId()))
1735+
if (value.tokvalue->exprId() == 0)
1736+
continue;
1737+
const ValueFlow::Value* v_p = utils::as_const(*pm).getValue(value.tokvalue->exprId());
1738+
if (!v_p)
17311739
continue;
1732-
const ValueFlow::Value& v_ref = utils::as_const(*pm).at(value.tokvalue->exprId());
1733-
if (!v_ref.isIntValue() && value.intvalue != 0)
1740+
if (!v_p->isIntValue() && value.intvalue != 0)
17341741
continue;
1735-
ValueFlow::Value v2 = v_ref;
1742+
ValueFlow::Value v2 = *v_p;
17361743
v2.intvalue += value.intvalue;
17371744
return v2;
17381745
}

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