Skip to content

Commit 31fa846

Browse files
firewavefirewave
committed
ProgramMemory: avoid duplicated lookups / removed at() 
1 parent 077e90a commit 31fa846

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
bool ProgramMemory::getIntValue(nonneg int exprid, MathLib::bigint& result, bool impossible) const
100115
{
101116
const ValueFlow::Value* value = getValue(exprid, impossible);
@@ -171,24 +186,6 @@ bool ProgramMemory::hasValue(nonneg int exprid, bool impossible) const
171186
return it != mValues->cend() && (impossible || !it->second.isImpossible());
172187
}
173188

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

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

13671365
ValueFlow::Value executeMultiCondition(bool b, const Token* expr)
13681366
{
1369-
if (pm->hasValue(expr->exprId())) {
1370-
const ValueFlow::Value& v = utils::as_const(*pm).at(expr->exprId());
1371-
if (v.isIntValue())
1372-
return v;
1367+
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId(), true)) {
1368+
if (v->isIntValue())
1369+
return *v;
13731370
}
13741371

13751372
// Evaluate recursively if there are no exprids
@@ -1498,18 +1495,18 @@ namespace {
14981495
if (rhs.isUninitValue())
14991496
return unknown();
15001497
if (expr->str() != "=") {
1501-
if (!pm->hasValue(expr->astOperand1()->exprId()))
1498+
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId(), true);
1499+
if (!lhs)
15021500
return unknown();
1503-
ValueFlow::Value& lhs = pm->at(expr->astOperand1()->exprId());
1504-
rhs = evaluate(removeAssign(expr->str()), lhs, rhs);
1505-
if (lhs.isIntValue())
1506-
ValueFlow::Value::visitValue(rhs, std::bind(assign{}, std::ref(lhs.intvalue), std::placeholders::_1));
1507-
else if (lhs.isFloatValue())
1501+
rhs = evaluate(removeAssign(expr->str()), *lhs, rhs);
1502+
if (lhs->isIntValue())
1503+
ValueFlow::Value::visitValue(rhs, std::bind(assign{}, std::ref(lhs->intvalue), std::placeholders::_1));
1504+
else if (lhs->isFloatValue())
15081505
ValueFlow::Value::visitValue(rhs,
1509-
std::bind(assign{}, std::ref(lhs.floatValue), std::placeholders::_1));
1506+
std::bind(assign{}, std::ref(lhs->floatValue), std::placeholders::_1));
15101507
else
15111508
return unknown();
1512-
return lhs;
1509+
return *lhs;
15131510
}
15141511
pm->setValue(expr->astOperand1(), rhs);
15151512
return rhs;
@@ -1521,20 +1518,20 @@ namespace {
15211518
execute(expr->astOperand1());
15221519
return execute(expr->astOperand2());
15231520
} else if (expr->tokType() == Token::eIncDecOp && expr->astOperand1() && expr->astOperand1()->exprId() != 0) {
1524-
if (!pm->hasValue(expr->astOperand1()->exprId()))
1521+
ValueFlow::Value* lhs = pm->getValue(expr->astOperand1()->exprId(), true);
1522+
if (!lhs)
15251523
return ValueFlow::Value::unknown();
1526-
ValueFlow::Value& lhs = pm->at(expr->astOperand1()->exprId());
1527-
if (!lhs.isIntValue())
1524+
if (!lhs->isIntValue())
15281525
return unknown();
15291526
// overflow
1530-
if (!lhs.isImpossible() && lhs.intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
1527+
if (!lhs->isImpossible() && lhs->intvalue == 0 && expr->str() == "--" && astIsUnsigned(expr->astOperand1()))
15311528
return unknown();
15321529

15331530
if (expr->str() == "++")
1534-
lhs.intvalue++;
1531+
lhs->intvalue++;
15351532
else
1536-
lhs.intvalue--;
1537-
return lhs;
1533+
lhs->intvalue--;
1534+
return *lhs;
15381535
} else if (expr->str() == "[" && expr->astOperand1() && expr->astOperand2()) {
15391536
const Token* tokvalue = nullptr;
15401537
if (!pm->getTokValue(expr->astOperand1()->exprId(), tokvalue)) {
@@ -1625,13 +1622,16 @@ namespace {
16251622
}
16261623
return execute(expr->astOperand1());
16271624
}
1628-
if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) {
1629-
ValueFlow::Value result = utils::as_const(*pm).at(expr->exprId());
1630-
if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
1631-
result.intvalue = !result.intvalue;
1632-
result.setKnown();
1625+
if (expr->exprId() > 0) {
1626+
if (const ValueFlow::Value* v = utils::as_const(*pm).getValue(expr->exprId(), true))
1627+
{
1628+
ValueFlow::Value result = *v;
1629+
if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
1630+
result.intvalue = !result.intvalue;
1631+
result.setKnown();
1632+
}
1633+
return result;
16331634
}
1634-
return result;
16351635
}
16361636

16371637
if (Token::Match(expr->previous(), ">|%name% {|(")) {
@@ -1681,14 +1681,16 @@ namespace {
16811681
}
16821682
// Check if function modifies argument
16831683
visitAstNodes(expr->astOperand2(), [&](const Token* child) {
1684-
if (child->exprId() > 0 && pm->hasValue(child->exprId())) {
1685-
ValueFlow::Value& v = pm->at(child->exprId());
1686-
if (v.valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE) {
1687-
if (ValueFlow::isContainerSizeChanged(child, v.indirect, settings))
1688-
v = unknown();
1689-
} else if (v.valueType != ValueFlow::Value::ValueType::UNINIT) {
1690-
if (isVariableChanged(child, v.indirect, settings))
1691-
v = unknown();
1684+
if (child->exprId() > 0) {
1685+
if (ValueFlow::Value* v = pm->getValue(child->exprId(), true))
1686+
{
1687+
if (v->valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE) {
1688+
if (ValueFlow::isContainerSizeChanged(child, v->indirect, settings))
1689+
*v = unknown();
1690+
} else if (v->valueType != ValueFlow::Value::ValueType::UNINIT) {
1691+
if (isVariableChanged(child, v->indirect, settings))
1692+
*v = unknown();
1693+
}
16921694
}
16931695
}
16941696
return ChildrenToVisit::op1_and_op2;
@@ -1740,22 +1742,27 @@ namespace {
17401742
return v;
17411743
if (!expr)
17421744
return v;
1743-
if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) {
1744-
if (updateValue(v, utils::as_const(*pm).at(expr->exprId())))
1745-
return v;
1745+
if (expr->exprId() > 0) {
1746+
if (const ValueFlow::Value* val = utils::as_const(*pm).getValue(expr->exprId(), true))
1747+
{
1748+
if (updateValue(v, *val))
1749+
return v;
1750+
}
17461751
}
17471752
// Find symbolic values
17481753
for (const ValueFlow::Value& value : expr->values()) {
17491754
if (!value.isSymbolicValue())
17501755
continue;
17511756
if (!value.isKnown())
17521757
continue;
1753-
if (value.tokvalue->exprId() > 0 && !pm->hasValue(value.tokvalue->exprId()))
1758+
if (value.tokvalue->exprId() == 0)
1759+
continue;
1760+
const ValueFlow::Value* v_p = utils::as_const(*pm).getValue(value.tokvalue->exprId(), true);
1761+
if (!v_p)
17541762
continue;
1755-
const ValueFlow::Value& v_ref = utils::as_const(*pm).at(value.tokvalue->exprId());
1756-
if (!v_ref.isIntValue() && value.intvalue != 0)
1763+
if (!v_p->isIntValue() && value.intvalue != 0)
17571764
continue;
1758-
ValueFlow::Value v2 = v_ref;
1765+
ValueFlow::Value v2 = *v_p;
17591766
v2.intvalue += value.intvalue;
17601767
return v2;
17611768
}

‎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
@@ -35,7 +35,6 @@ class TestProgramMemory : public TestFixture {
3535
TEST_CASE(copyOnWrite);
3636
TEST_CASE(hasValue);
3737
TEST_CASE(getValue);
38-
TEST_CASE(at);
3938
}
4039

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

4746
ProgramMemory pm;
48-
const ValueFlow::Value* v = pm.getValue(id, false);
47+
const ValueFlow::Value* v = utils::as_const(pm).getValue(id, false);
4948
ASSERT(!v);
5049
pm.setValue(tok, ValueFlow::Value{41});
5150

52-
v = pm.getValue(id, false);
51+
v = utils::as_const(pm).getValue(id, false);
5352
ASSERT(v);
5453
ASSERT_EQUALS(41, v->intvalue);
5554

5655
// create a copy
5756
ProgramMemory pm2 = pm;
5857

5958
// make sure the value was copied
60-
v = pm2.getValue(id, false);
59+
v = utils::as_const(pm2).getValue(id, false);
6160
ASSERT(v);
6261
ASSERT_EQUALS(41, v->intvalue);
6362

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

7372
// make sure the value was set
74-
v = pm2.getValue(id, false);
73+
v = utils::as_const(pm2).getValue(id, false);
7574
ASSERT(v);
7675
ASSERT_EQUALS(42, v->intvalue);
7776

7877
// make sure the value was set
79-
v = pm3.getValue(id, false);
78+
v = utils::as_const(pm3).getValue(id, false);
8079
ASSERT(v);
8180
ASSERT_EQUALS(43, v->intvalue);
8281

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

9493
void getValue() const {
9594
ProgramMemory pm;
96-
ASSERT(!pm.getValue(123, false));
97-
}
98-
99-
void at() const {
100-
ProgramMemory pm;
101-
ASSERT_THROW_EQUALS_2(pm.at(123), std::out_of_range, "ProgramMemory::at");
102-
ASSERT_THROW_EQUALS_2(utils::as_const(pm).at(123), std::out_of_range, "ProgramMemory::at");
95+
ASSERT(!utils::as_const(pm).getValue(123, false));
10396
}
10497
};
10598

0 commit comments

Comments
 (0)