Skip to content

Commit 1aaf14f

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

File tree

3 files changed

+74
-76
lines changed

3 files changed

+74
-76
lines changed

‎lib/programmemory.cpp‎

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

99+
ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossible)
100+
{
101+
if (mValues->empty())
102+
return nullptr;
103+
104+
// TODO: avoid copy if no value is found
105+
copyOnWrite();
106+
107+
const auto it = find(exprid);
108+
const bool found = it != mValues->cend() && (impossible || !it->second.isImpossible());
109+
if (found)
110+
return &it->second;
111+
return nullptr;
112+
}
113+
99114
// cppcheck-suppress unusedFunction
100115
bool ProgramMemory::getIntValue(nonneg int exprid, MathLib::bigint& result, bool impossible) const
101116
{
@@ -172,24 +187,6 @@ bool ProgramMemory::hasValue(nonneg int exprid, bool impossible) const
172187
return it != mValues->cend() && (impossible || !it->second.isImpossible());
173188
}
174189

175-
const ValueFlow::Value& ProgramMemory::at(nonneg int exprid, bool impossible) const {
176-
const auto it = find(exprid);
177-
if (it == mValues->cend() && (impossible || !it->second.isImpossible())) {
178-
throw std::out_of_range("ProgramMemory::at");
179-
}
180-
return it->second;
181-
}
182-
183-
ValueFlow::Value& ProgramMemory::at(nonneg int exprid, bool impossible) {
184-
copyOnWrite();
185-
186-
const auto it = find(exprid);
187-
if (it == mValues->end() && (impossible || !it->second.isImpossible())) {
188-
throw std::out_of_range("ProgramMemory::at");
189-
}
190-
return it->second;
191-
}
192-
193190
void ProgramMemory::erase_if(const std::function<bool(const ExprIdToken&)>& pred)
194191
{
195192
if (mValues->empty())
@@ -253,6 +250,7 @@ ProgramMemory::Map::const_iterator ProgramMemory::find(nonneg int exprid) const
253250
return cvalues.find(ExprIdToken::create(exprid));
254251
}
255252

253+
// need to call copyOnWrite() before calling this
256254
ProgramMemory::Map::iterator ProgramMemory::find(nonneg int exprid)
257255
{
258256
return mValues->find(ExprIdToken::create(exprid));
@@ -1350,10 +1348,9 @@ namespace {
13501348

13511349
ValueFlow::Value executeMultiCondition(bool b, const Token* expr)
13521350
{
1353-
if (pm->hasValue(expr->exprId())) {
1354-
const ValueFlow::Value& v = utils::as_const(*pm).at(expr->exprId());
1355-
if (v.isIntValue())
1356-
return v;
1351+
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId(), true)) {
1352+
if (v->isIntValue())
1353+
return *v;
13571354
}
13581355

13591356
// Evaluate recursively if there are no exprids
@@ -1482,18 +1479,18 @@ namespace {
14821479
if (rhs.isUninitValue())
14831480
return unknown();
14841481
if (expr->str() != "=") {
1485-
if (!pm->hasValue(expr->astOperand1()->exprId()))
1482+
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId(), true);
1483+
if (!lhs)
14861484
return unknown();
1487-
ValueFlow::Value& lhs = pm->at(expr->astOperand1()->exprId());
1488-
rhs = evaluate(removeAssign(expr->str()), lhs, rhs);
1489-
if (lhs.isIntValue())
1490-
ValueFlow::Value::visitValue(rhs, std::bind(assign{}, std::ref(lhs.intvalue), std::placeholders::_1));
1491-
else if (lhs.isFloatValue())
1485+
rhs = evaluate(removeAssign(expr->str()), *lhs, rhs);
1486+
if (lhs->isIntValue())
1487+
ValueFlow::Value::visitValue(rhs, std::bind(assign{}, std::ref(lhs->intvalue), std::placeholders::_1));
1488+
else if (lhs->isFloatValue())
14921489
ValueFlow::Value::visitValue(rhs,
1493-
std::bind(assign{}, std::ref(lhs.floatValue), std::placeholders::_1));
1490+
std::bind(assign{}, std::ref(lhs->floatValue), std::placeholders::_1));
14941491
else
14951492
return unknown();
1496-
return lhs;
1493+
return *lhs;
14971494
}
14981495
pm->setValue(expr->astOperand1(), rhs);
14991496
return rhs;
@@ -1505,20 +1502,20 @@ namespace {
15051502
execute(expr->astOperand1());
15061503
return execute(expr->astOperand2());
15071504
} else if (expr->tokType() == Token::eIncDecOp && expr->astOperand1() && expr->astOperand1()->exprId() != 0) {
1508-
if (!pm->hasValue(expr->astOperand1()->exprId()))
1505+
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId(), true);
1506+
if (!lhs)
15091507
return ValueFlow::Value::unknown();
1510-
ValueFlow::Value& lhs = pm->at(expr->astOperand1()->exprId());
1511-
if (!lhs.isIntValue())
1508+
if (!lhs->isIntValue())
15121509
return unknown();
15131510
// overflow
1514-
if (!lhs.isImpossible() && lhs.intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
1511+
if (!lhs->isImpossible() && lhs->intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
15151512
return unknown();
15161513

15171514
if (expr->str() == "++")
1518-
lhs.intvalue++;
1515+
lhs->intvalue++;
15191516
else
1520-
lhs.intvalue--;
1521-
return lhs;
1517+
lhs->intvalue--;
1518+
return *lhs;
15221519
} else if (expr->str() == "[" && expr->astOperand1() && expr->astOperand2()) {
15231520
const Token* tokvalue = nullptr;
15241521
if (!pm->getTokValue(expr->astOperand1()->exprId(), tokvalue)) {
@@ -1609,13 +1606,16 @@ namespace {
16091606
}
16101607
return execute(expr->astOperand1());
16111608
}
1612-
if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) {
1613-
ValueFlow::Value result = utils::as_const(*pm).at(expr->exprId());
1614-
if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
1615-
result.intvalue = !result.intvalue;
1616-
result.setKnown();
1609+
if (expr->exprId() > 0) {
1610+
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId(), true))
1611+
{
1612+
ValueFlow::Value result = *v;
1613+
if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
1614+
result.intvalue = !result.intvalue;
1615+
result.setKnown();
1616+
}
1617+
return result;
16171618
}
1618-
return result;
16191619
}
16201620

16211621
if (Token::Match(expr->previous(), ">|%name% {|(")) {
@@ -1665,14 +1665,16 @@ namespace {
16651665
}
16661666
// Check if function modifies argument
16671667
visitAstNodes(expr->astOperand2(), [&](const Token* child) {
1668-
if (child->exprId() > 0 && pm->hasValue(child->exprId())) {
1669-
ValueFlow::Value& v = pm->at(child->exprId());
1670-
if (v.valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE) {
1671-
if (ValueFlow::isContainerSizeChanged(child, v.indirect, settings))
1672-
v = unknown();
1673-
} else if (v.valueType != ValueFlow::Value::ValueType::UNINIT) {
1674-
if (isVariableChanged(child, v.indirect, settings))
1675-
v = unknown();
1668+
if (child->exprId() > 0) {
1669+
if (ValueFlow::Value* v = pm->getValue(child->exprId(), true))
1670+
{
1671+
if (v->valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE) {
1672+
if (ValueFlow::isContainerSizeChanged(child, v->indirect, settings))
1673+
*v = unknown();
1674+
} else if (v->valueType != ValueFlow::Value::ValueType::UNINIT) {
1675+
if (isVariableChanged(child, v->indirect, settings))
1676+
*v = unknown();
1677+
}
16761678
}
16771679
}
16781680
return ChildrenToVisit::op1_and_op2;
@@ -1724,22 +1726,27 @@ namespace {
17241726
return v;
17251727
if (!expr)
17261728
return v;
1727-
if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) {
1728-
if (updateValue(v, utils::as_const(*pm).at(expr->exprId())))
1729-
return v;
1729+
if (expr->exprId() > 0) {
1730+
if (const ValueFlow::Value* val = utils::as_const(*pm).getValue(expr->exprId(), true))
1731+
{
1732+
if (updateValue(v, *val))
1733+
return v;
1734+
}
17301735
}
17311736
// Find symbolic values
17321737
for (const ValueFlow::Value& value : expr->values()) {
17331738
if (!value.isSymbolicValue())
17341739
continue;
17351740
if (!value.isKnown())
17361741
continue;
1737-
if (value.tokvalue->exprId() > 0 && !pm->hasValue(value.tokvalue->exprId()))
1742+
if (value.tokvalue->exprId() == 0)
1743+
continue;
1744+
const ValueFlow::Value* v_p = utils::as_const(*pm).getValue(value.tokvalue->exprId(), true);
1745+
if (!v_p)
17381746
continue;
1739-
const ValueFlow::Value& v_ref = utils::as_const(*pm).at(value.tokvalue->exprId());
1740-
if (!v_ref.isIntValue() && value.intvalue != 0)
1747+
if (!v_p->isIntValue() && value.intvalue != 0)
17411748
continue;
1742-
ValueFlow::Value v2 = v_ref;
1749+
ValueFlow::Value v2 = *v_p;
17431750
v2.intvalue += value.intvalue;
17441751
return v2;
17451752
}

‎lib/programmemory.h‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,8 @@ struct CPPCHECKLIB ProgramMemory {
109109
explicit ProgramMemory(Map values) : mValues(new Map(std::move(values))) {}
110110

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

114115
bool getIntValue(nonneg int exprid, MathLib::bigint& result, bool impossible = false) 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, bool impossible = false) const;
124125
bool hasValue(nonneg int exprid, bool impossible = true) const;
125126

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

131129
void swap(ProgramMemory &pm) NOEXCEPT;

‎test/testprogrammemory.cpp‎

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ class TestProgramMemory : public TestFixture {
3232
TEST_CASE(copyOnWrite);
3333
TEST_CASE(hasValue);
3434
TEST_CASE(getValue);
35-
TEST_CASE(at);
3635
}
3736

3837
void copyOnWrite() const {
@@ -42,19 +41,19 @@ class TestProgramMemory : public TestFixture {
4241
tok->exprId(id);
4342

4443
ProgramMemory pm;
45-
const ValueFlow::Value* v = pm.getValue(id, false);
44+
const ValueFlow::Value* v = utils::as_const(pm).getValue(id, false);
4645
ASSERT(!v);
4746
pm.setValue(tok, ValueFlow::Value{41});
4847

49-
v = pm.getValue(id, false);
48+
v = utils::as_const(pm).getValue(id, false);
5049
ASSERT(v);
5150
ASSERT_EQUALS(41, v->intvalue);
5251

5352
// create a copy
5453
ProgramMemory pm2 = pm;
5554

5655
// make sure the value was copied
57-
v = pm2.getValue(id, false);
56+
v = utils::as_const(pm2).getValue(id, false);
5857
ASSERT(v);
5958
ASSERT_EQUALS(41, v->intvalue);
6059

@@ -68,17 +67,17 @@ class TestProgramMemory : public TestFixture {
6867
pm3.setValue(tok, ValueFlow::Value{43});
6968

7069
// make sure the value was set
71-
v = pm2.getValue(id, false);
70+
v = utils::as_const(pm2).getValue(id, false);
7271
ASSERT(v);
7372
ASSERT_EQUALS(42, v->intvalue);
7473

7574
// make sure the value was set
76-
v = pm3.getValue(id, false);
75+
v = utils::as_const(pm3).getValue(id, false);
7776
ASSERT(v);
7877
ASSERT_EQUALS(43, v->intvalue);
7978

8079
// make sure the original value remains unchanged
81-
v = pm.getValue(id, false);
80+
v = utils::as_const(pm).getValue(id, false);
8281
ASSERT(v);
8382
ASSERT_EQUALS(41, v->intvalue);
8483
}
@@ -90,13 +89,7 @@ class TestProgramMemory : public TestFixture {
9089

9190
void getValue() const {
9291
ProgramMemory pm;
93-
ASSERT(!pm.getValue(123, false));
94-
}
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");
92+
ASSERT(!utils::as_const(pm).getValue(123, false));
10093
}
10194
};
10295

0 commit comments

Comments
 (0)