Skip to content

Commit 45de9af

Browse files
danmardmcppcheck
danmar
and
dmcppcheck
authored
Fix #722 (#if: &&, || and ?: do not short-circuit) (#723)
Co-authored-by: Daniel Marjamäki <daniel.marjamaki@cppcheck.com>
1 parent b833a75 commit 45de9af

2 files changed

Lines changed: 188 additions & 56 deletions

File tree

‎simplecpp.cpp‎

Lines changed: 135 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -179,14 +179,31 @@ static bool isValidPunctuatorPaste(const std::string &s)
179179
return isPunctuator(s) || (s.size() >= 2 && s[0] == '.' && std::isdigit(static_cast<unsigned char>(s[1])));
180180
}
181181

182+
/*
183+
* When an operation can't be evaluated (division by zero), its result is an
184+
* error value. The error is only reported if the error value is used, so an
185+
* operand that is not evaluated, as in "0 && (1/0)", does not cause an error.
186+
*/
187+
static const std::string ERROR_VALUE("#error:");
188+
189+
static bool isErrorValue(const simplecpp::Token *tok)
190+
{
191+
return tok && tok->str().compare(0, ERROR_VALUE.size(), ERROR_VALUE) == 0;
192+
}
193+
194+
static bool isValue(const simplecpp::Token *tok)
195+
{
196+
return tok && (tok->number || isErrorValue(tok));
197+
}
198+
182199
static bool isAlternativeBinaryOp(const simplecpp::Token *tok, const std::string &alt)
183200
{
184201
return (tok->name &&
185202
tok->str() == alt &&
186203
tok->previous &&
187204
tok->next &&
188-
(tok->previous->number || tok->previous->name || tok->previous->op == ')') &&
189-
(tok->next->number || tok->next->name || tok->next->op == '('));
205+
(isValue(tok->previous) || tok->previous->name || tok->previous->op == ')') &&
206+
(isValue(tok->next) || tok->next->name || tok->next->op == '('));
190207
}
191208

192209
static bool isAlternativeUnaryOp(const simplecpp::Token *tok, const std::string &alt)
@@ -1072,6 +1089,23 @@ void simplecpp::TokenList::readfile(Stream &stream, const std::string &filename,
10721089
combineOperators();
10731090
}
10741091

1092+
/**
1093+
* If one operand of the binary operator tok is an error value then
1094+
* "lhs op rhs" is replaced by that error value.
1095+
* @return the error value token, or nullptr
1096+
*/
1097+
static simplecpp::Token *foldErrorValue(simplecpp::TokenList &tokens, simplecpp::Token *tok)
1098+
{
1099+
simplecpp::Token * const lhs = tok->previous;
1100+
if (!isErrorValue(lhs) && !isErrorValue(tok->next))
1101+
return nullptr;
1102+
if (!isErrorValue(lhs))
1103+
lhs->setstr(tok->next->str());
1104+
tokens.deleteToken(tok->next);
1105+
tokens.deleteToken(tok);
1106+
return lhs;
1107+
}
1108+
10751109
void simplecpp::TokenList::constFold()
10761110
{
10771111
while (cfront()) {
@@ -1105,6 +1139,11 @@ void simplecpp::TokenList::constFold()
11051139
deleteToken(tok->previous);
11061140
deleteToken(tok->next);
11071141
}
1142+
1143+
for (const Token *tok = cfront(); tok; tok = tok->next) {
1144+
if (isErrorValue(tok))
1145+
throw std::overflow_error(tok->str().substr(ERROR_VALUE.size()));
1146+
}
11081147
}
11091148

11101149
static bool isFloatSuffix(const simplecpp::Token *tok)
@@ -1262,17 +1301,27 @@ void simplecpp::TokenList::constFoldUnaryNotPosNeg(simplecpp::Token *tok)
12621301
else if (isAlternativeUnaryOp(tok, COMPL))
12631302
tok->op = '~';
12641303

1265-
if (tok->op == '!' && tok->next && tok->next->number) {
1304+
if (tok->isOneOf("!~") && isErrorValue(tok->next)) {
1305+
tok->setstr(tok->next->str());
1306+
deleteToken(tok->next);
1307+
} else if (tok->op == '!' && tok->next && tok->next->number) {
12661308
tok->setstr(tok->next->str() == "0" ? "1" : "0");
12671309
deleteToken(tok->next);
12681310
} else if (tok->op == '~' && tok->next && tok->next->number) {
12691311
tok->setstr(toString(~stringToLL(tok->next->str())));
12701312
deleteToken(tok->next);
12711313
} else {
1272-
if (tok->previous && (tok->previous->number || tok->previous->name))
1314+
if (tok->previous && (isValue(tok->previous) || tok->previous->name))
12731315
continue;
1274-
if (!tok->next || !tok->next->number)
1316+
if (!isValue(tok->next))
12751317
continue;
1318+
if (isErrorValue(tok->next)) {
1319+
if (tok->isOneOf("+-")) {
1320+
tok->setstr(tok->next->str());
1321+
deleteToken(tok->next);
1322+
}
1323+
continue;
1324+
}
12761325
switch (tok->op) {
12771326
case '+':
12781327
tok->setstr(tok->next->str());
@@ -1290,32 +1339,33 @@ void simplecpp::TokenList::constFoldUnaryNotPosNeg(simplecpp::Token *tok)
12901339
void simplecpp::TokenList::constFoldMulDivRem(Token *tok)
12911340
{
12921341
for (; tok && tok->op != ')'; tok = tok->next) {
1293-
if (!tok->previous || !tok->previous->number)
1342+
if (!isValue(tok->previous) || !isValue(tok->next))
1343+
continue;
1344+
if (!tok->isOneOf("*/%"))
12941345
continue;
1295-
if (!tok->next || !tok->next->number)
1346+
if (Token * const errorValue = foldErrorValue(*this, tok)) {
1347+
tok = errorValue;
12961348
continue;
1349+
}
12971350

1298-
long long result;
1351+
std::string result;
12991352
if (tok->op == '*') {
1300-
result = (stringToLL(tok->previous->str()) * stringToLL(tok->next->str()));
1301-
}
1302-
else if (tok->op == '/' || tok->op == '%') {
1353+
result = toString(stringToLL(tok->previous->str()) * stringToLL(tok->next->str()));
1354+
} else {
13031355
const long long rhs = stringToLL(tok->next->str());
1304-
if (rhs == 0)
1305-
throw std::overflow_error("division/modulo by zero");
13061356
const long long lhs = stringToLL(tok->previous->str());
1307-
if (rhs == -1 && lhs == std::numeric_limits<long long>::min())
1308-
throw std::overflow_error("division overflow");
1309-
if (tok->op == '/')
1310-
result = (lhs / rhs);
1357+
if (rhs == 0)
1358+
result = ERROR_VALUE + "division/modulo by zero";
1359+
else if (rhs == -1 && lhs == std::numeric_limits<long long>::min())
1360+
result = ERROR_VALUE + "division overflow";
1361+
else if (tok->op == '/')
1362+
result = toString(lhs / rhs);
13111363
else
1312-
result = (lhs % rhs);
1313-
} else {
1314-
continue;
1364+
result = toString(lhs % rhs);
13151365
}
13161366

13171367
tok = tok->previous;
1318-
tok->setstr(toString(result));
1368+
tok->setstr(result);
13191369
deleteToken(tok->next);
13201370
deleteToken(tok->next);
13211371
}
@@ -1324,10 +1374,14 @@ void simplecpp::TokenList::constFoldMulDivRem(Token *tok)
13241374
void simplecpp::TokenList::constFoldAddSub(Token *tok)
13251375
{
13261376
for (; tok && tok->op != ')'; tok = tok->next) {
1327-
if (!tok->previous || !tok->previous->number)
1377+
if (!isValue(tok->previous) || !isValue(tok->next))
1378+
continue;
1379+
if (!tok->isOneOf("+-"))
13281380
continue;
1329-
if (!tok->next || !tok->next->number)
1381+
if (Token * const errorValue = foldErrorValue(*this, tok)) {
1382+
tok = errorValue;
13301383
continue;
1384+
}
13311385

13321386
long long result;
13331387
if (tok->op == '+')
@@ -1347,10 +1401,14 @@ void simplecpp::TokenList::constFoldAddSub(Token *tok)
13471401
void simplecpp::TokenList::constFoldShift(Token *tok)
13481402
{
13491403
for (; tok && tok->op != ')'; tok = tok->next) {
1350-
if (!tok->previous || !tok->previous->number)
1404+
if (!isValue(tok->previous) || !isValue(tok->next))
1405+
continue;
1406+
if (tok->str() != "<<" && tok->str() != ">>")
13511407
continue;
1352-
if (!tok->next || !tok->next->number)
1408+
if (Token * const errorValue = foldErrorValue(*this, tok)) {
1409+
tok = errorValue;
13531410
continue;
1411+
}
13541412

13551413
long long result;
13561414
if (tok->str() == "<<")
@@ -1374,12 +1432,14 @@ void simplecpp::TokenList::constFoldComparison(Token *tok)
13741432
if (isAlternativeBinaryOp(tok,NOTEQ))
13751433
tok->setstr("!=");
13761434

1377-
if (!tok->startsWithOneOf("<>=!"))
1435+
if (!tok->isOneOf("<>") && tok->str() != "<=" && tok->str() != ">=" && tok->str() != "==" && tok->str() != "!=")
13781436
continue;
1379-
if (!tok->previous || !tok->previous->number)
1437+
if (!isValue(tok->previous) || !isValue(tok->next))
13801438
continue;
1381-
if (!tok->next || !tok->next->number)
1439+
if (Token * const errorValue = foldErrorValue(*this, tok)) {
1440+
tok = errorValue;
13821441
continue;
1442+
}
13831443

13841444
int result;
13851445
if (tok->str() == "==")
@@ -1419,10 +1479,12 @@ void simplecpp::TokenList::constFoldBitwise(Token *tok)
14191479
for (tok = tok1; tok && tok->op != ')'; tok = tok->next) {
14201480
if (tok->op != *op && !isAlternativeBinaryOp(tok, *alternativeOp))
14211481
continue;
1422-
if (!tok->previous || !tok->previous->number)
1482+
if (!isValue(tok->previous) || !isValue(tok->next))
14231483
continue;
1424-
if (!tok->next || !tok->next->number)
1484+
if (Token * const errorValue = foldErrorValue(*this, tok)) {
1485+
tok = errorValue;
14251486
continue;
1487+
}
14261488
long long result;
14271489
if (*op == '&')
14281490
result = (stringToLL(tok->previous->str()) & stringToLL(tok->next->str()));
@@ -1455,19 +1517,24 @@ void simplecpp::TokenList::constFoldLogicalOp(Token *tok)
14551517
for (Token *t = tok; t && t->op != ')'; t = t->next) {
14561518
if (t->str() != op)
14571519
continue;
1458-
if (!t->previous || !t->previous->number)
1459-
continue;
1460-
if (!t->next || !t->next->number)
1520+
if (!isValue(t->previous) || !isValue(t->next))
14611521
continue;
14621522

1463-
int result;
1464-
if (t->str() == "||")
1465-
result = (stringToLL(t->previous->str()) || stringToLL(t->next->str()));
1466-
else /*if (t->str() == "&&")*/
1467-
result = (stringToLL(t->previous->str()) && stringToLL(t->next->str()));
1523+
// the right operand is not evaluated if the left operand decides the result
1524+
std::string result;
1525+
if (isErrorValue(t->previous))
1526+
result = t->previous->str();
1527+
else if (t->str() == "||" && stringToLL(t->previous->str()) != 0)
1528+
result = "1";
1529+
else if (t->str() == "&&" && stringToLL(t->previous->str()) == 0)
1530+
result = "0";
1531+
else if (isErrorValue(t->next))
1532+
result = t->next->str();
1533+
else
1534+
result = (stringToLL(t->next->str()) != 0) ? "1" : "0";
14681535

14691536
t = t->previous;
1470-
t->setstr(toString(result));
1537+
t->setstr(result);
14711538
deleteToken(t->next);
14721539
deleteToken(t->next);
14731540
}
@@ -1476,30 +1543,42 @@ void simplecpp::TokenList::constFoldLogicalOp(Token *tok)
14761543

14771544
void simplecpp::TokenList::constFoldQuestionOp(Token *&tok1)
14781545
{
1479-
bool gotoTok1 = false;
1480-
// NOLINTNEXTLINE(misc-const-correctness) - technically correct but used to access non-const data
1481-
for (Token *tok = tok1; tok && tok->op != ')'; tok = gotoTok1 ? tok1 : tok->next) {
1482-
gotoTok1 = false;
1483-
if (tok->str() != "?")
1484-
continue;
1546+
// ?: is right-associative, "a ? b : c ? d : e" means "a ? b : (c ? d : e)",
1547+
// so the last ? is folded first
1548+
std::vector<const Token *> questionTokens;
1549+
for (const Token *t = tok1; t && t->op != ')'; t = t->next) {
1550+
if (t->str() == "?")
1551+
questionTokens.push_back(t);
1552+
}
1553+
while (!questionTokens.empty()) {
1554+
const Token * const tok = questionTokens.back();
1555+
questionTokens.pop_back();
14851556
if (!tok->previous || !tok->next || !tok->next->next)
14861557
throw std::runtime_error("invalid expression");
1487-
if (!tok->previous->number)
1488-
continue;
1489-
if (tok->next->next->op != ':')
1490-
continue;
14911558
Token * const condTok = tok->previous;
14921559
Token * const trueTok = tok->next;
14931560
Token * const falseTok = trueTok->next->next;
14941561
if (!falseTok)
14951562
throw std::runtime_error("invalid expression");
1496-
if (condTok == tok1)
1497-
tok1 = (condTok->str() != "0" ? trueTok : falseTok);
1498-
deleteToken(condTok->next); // ?
1499-
deleteToken(trueTok->next); // :
1500-
deleteToken(condTok->str() == "0" ? trueTok : falseTok);
1501-
deleteToken(condTok);
1502-
gotoTok1 = true;
1563+
if (trueTok->next->op != ':')
1564+
return;
1565+
if (!isValue(condTok) || !isValue(trueTok) || !isValue(falseTok))
1566+
return;
1567+
if (isErrorValue(condTok)) {
1568+
// "error ? x : y" => error
1569+
deleteToken(condTok->next); // ?
1570+
deleteToken(trueTok->next); // :
1571+
deleteToken(trueTok);
1572+
deleteToken(falseTok);
1573+
} else {
1574+
Token * const result = (condTok->str() != "0" ? trueTok : falseTok);
1575+
if (condTok == tok1)
1576+
tok1 = result;
1577+
deleteToken(condTok->next); // ?
1578+
deleteToken(trueTok->next); // :
1579+
deleteToken(result == falseTok ? trueTok : falseTok); // the operand that is not evaluated
1580+
deleteToken(condTok);
1581+
}
15031582
}
15041583
}
15051584

‎test.cpp‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,6 +620,22 @@ static void constFold()
620620
ASSERT_EQUALS("1", testConstFold("010==8"));
621621
ASSERT_EQUALS("exception", testConstFold("!1 ? 2 :"));
622622
ASSERT_EQUALS("exception", testConstFold("?2:3"));
623+
ASSERT_EQUALS("0", testConstFold("0 && 1/0"));
624+
ASSERT_EQUALS("1", testConstFold("1 || 1/0"));
625+
ASSERT_EQUALS("2", testConstFold("1 ? 2 : 1/0"));
626+
ASSERT_EQUALS("exception", testConstFold("1 && 1/0"));
627+
ASSERT_EQUALS("exception", testConstFold("1/0 && 0"));
628+
ASSERT_EQUALS("exception", testConstFold("(1/0) ? 1 : 2"));
629+
ASSERT_EQUALS("exception", testConstFold("-(1/0) + 1"));
630+
// ?: is right-associative
631+
ASSERT_EQUALS("1", testConstFold("1 ? 1 : 0 ? 2 : 3"));
632+
ASSERT_EQUALS("3", testConstFold("0 ? 1 : 0 ? 2 : 3"));
633+
ASSERT_EQUALS("2", testConstFold("1 ? 2 : 3 ? 4 : 5 ? 6 : 7"));
634+
ASSERT_EQUALS("7", testConstFold("0 ? 2 : 0 ? 4 : 0 ? 6 : 7"));
635+
ASSERT_EQUALS("6", testConstFold("1 ? 0 ? 5 : 6 : 7"));
636+
ASSERT_EQUALS("2", testConstFold("(1 ? 1 : 0) ? 2 : 3"));
637+
ASSERT_EQUALS("1", testConstFold("1 ? 1 : (1/0) ? 2 : 3"));
638+
ASSERT_EQUALS("exception", testConstFold("0 ? 1 : (1/0) ? 2 : 3"));
623639
}
624640

625641
#ifdef __CYGWIN__
@@ -2835,6 +2851,42 @@ static void ifdiv0()
28352851
ASSERT_EQUALS("", preprocess(code));
28362852
}
28372853

2854+
static void ifShortCircuit()
2855+
{
2856+
// operands that are not evaluated must not cause errors
2857+
const char * const exprs[] = {
2858+
"0 && (1/0)",
2859+
"!(1 || (1/0))",
2860+
"0 ? 1/0 : 0",
2861+
"1 ? 0 : 1/0",
2862+
"0 && 1 % 0 && 1",
2863+
"(1 || 1/0) && !(0 && 1/0) && 0",
2864+
"0 && (2 + (3 * (1/0)))",
2865+
"0 && -(1/0)",
2866+
"0 and (1/0)",
2867+
"!(1 or 1/0)",
2868+
"defined(X) and X/0",
2869+
"0 && 1 bitor (1/0)",
2870+
"!(1 || (1/0) bitand 1)",
2871+
"0 and 1 xor (1/0)"
2872+
};
2873+
for (const char * const expr : exprs) {
2874+
const std::string code = std::string("#if ") + expr + "\n"
2875+
"1\n"
2876+
"#else\n"
2877+
"2\n"
2878+
"#endif\n";
2879+
simplecpp::OutputList outputList;
2880+
ASSERT_EQUALS("\n\n\n2", preprocess(code.c_str(), &outputList));
2881+
ASSERT_EQUALS("", toString(outputList));
2882+
}
2883+
2884+
// evaluated operands are still checked
2885+
simplecpp::OutputList outputList;
2886+
ASSERT_EQUALS("", preprocess("#if 1 && (1/0)\n#endif\n", &outputList));
2887+
ASSERT_EQUALS("file0,1,syntax_error,failed to evaluate #if condition, division/modulo by zero\n", toString(outputList));
2888+
}
2889+
28382890
static void ifTrueFalse()
28392891
{
28402892
const char code[] = "#if true && !false\n"
@@ -5070,6 +5122,7 @@ static void runTests(int argc, char **argv, Input input)
50705122
TEST_CASE(ifif);
50715123
TEST_CASE(ifoverflow);
50725124
TEST_CASE(ifdiv0);
5125+
TEST_CASE(ifShortCircuit);
50735126
TEST_CASE(ifTrueFalse);
50745127
TEST_CASE(ifalt); // using "and", "or", etc
50755128
TEST_CASE(ifexpr);

0 commit comments

Comments
 (0)