Skip to content

Commit b1d1869

Browse files
danmardanmar
committed
minor tweak of comment
1 parent 2532f94 commit b1d1869

5 files changed

Lines changed: 232 additions & 1 deletion

File tree

‎addons/cppcheckdata.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,7 @@ class ValueFlow:
352352
# http://cppcheck.sourceforge.net/devinfo/doxyoutput/classValueFlow_1_1Value.html
353353

354354
class Value:
355-
# #integer value
355+
## integer value
356356
intvalue = None
357357
## token value
358358
tokvalue = None

‎lib/astutils.cpp‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,51 @@ bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token
239239
(comp1 == ">" && comp2 == "<"))));
240240
}
241241

242+
243+
bool isIncorrectCond(bool isNot, bool cpp, const Token * const cond1, const Token * const cond2, const std::set<std::string> &constFunctions)
244+
{
245+
if (!cond1 || !cond2)
246+
return false;
247+
248+
if (cond1->str() == "!") {
249+
if (cond2->str() == "!=") {
250+
if (cond2->astOperand1() && cond2->astOperand1()->str() == "0")
251+
return isSameExpression(cpp, cond1->astOperand1(), cond2->astOperand2(), constFunctions);
252+
if (cond2->astOperand2() && cond2->astOperand2()->str() == "0")
253+
return isSameExpression(cpp, cond1->astOperand1(), cond2->astOperand1(), constFunctions);
254+
}
255+
return isSameExpression(cpp, cond1->astOperand1(), cond2, constFunctions);
256+
}
257+
258+
if (cond2->str() == "!")
259+
return isIncorrectCond(isNot, cpp, cond2, cond1, constFunctions);
260+
261+
if (!cond1->isComparisonOp() || !cond2->isComparisonOp())
262+
return false;
263+
264+
const std::string &comp1 = cond1->str();
265+
266+
// condition found .. get comparator
267+
std::string comp2;
268+
if (isSameExpression(cpp, cond1->astOperand1(), cond2->astOperand1(), constFunctions) &&
269+
isSameExpression(cpp, cond1->astOperand2(), cond2->astOperand2(), constFunctions)) {
270+
comp2 = cond2->str();
271+
} else if (isSameExpression(cpp, cond1->astOperand1(), cond2->astOperand2(), constFunctions) &&
272+
isSameExpression(cpp, cond1->astOperand2(), cond2->astOperand1(), constFunctions)) {
273+
comp2 = cond2->str();
274+
if (comp2[0] == '>')
275+
comp2[0] = '<';
276+
else if (comp2[0] == '<')
277+
comp2[0] = '>';
278+
}
279+
280+
// is condition incorrect?
281+
return ((comp1 == "<" && comp2 == "==") ||
282+
(comp1 == ">" && comp2 == "==") ||
283+
(comp1 == "==" && comp2 == "<") ||
284+
(comp1 == "==" && comp2 == ">"));
285+
}
286+
242287
bool isConstExpression(const Token *tok, const std::set<std::string> &constFunctions)
243288
{
244289
if (!tok)

‎lib/astutils.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ bool isSameExpression(bool cpp, const Token *tok1, const Token *tok2, const std:
5959
* @param constFunctions constFunctions
6060
*/
6161
bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token * const cond2, const std::set<std::string> &constFunctions);
62+
bool isIncorrectCond(bool isNot, bool cpp, const Token * const cond1, const Token * const cond2, const std::set<std::string> &constFunctions);
6263

6364
bool isConstExpression(const Token *tok, const std::set<std::string> &constFunctions);
6465

‎lib/checkcondition.cpp‎

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -641,6 +641,187 @@ static std::string conditionString(bool not1, const Token *expr1, const std::str
641641
(expr1->isName() ? expr1->str() : std::string("EXPR"));
642642
}
643643

644+
void CheckCondition::checkIncorrectCondition()
645+
{
646+
const bool printStyle = _settings->isEnabled("style");
647+
const bool printWarning = _settings->isEnabled("warning");
648+
if (!printWarning && !printStyle)
649+
return;
650+
651+
const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
652+
const std::size_t functions = symbolDatabase->functionScopes.size();
653+
for (std::size_t ii = 0; ii < functions; ++ii) {
654+
const Scope * scope = symbolDatabase->functionScopes[ii];
655+
656+
for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) {
657+
if (!Token::Match(tok, "%oror%|&&") || !tok->astOperand1() || !tok->astOperand2())
658+
continue;
659+
660+
// Opposite comparisons around || or && => always true or always false
661+
if ((tok->astOperand1()->isName() || tok->astOperand2()->isName()) &&
662+
isIncorrectCond(true, _tokenizer->isCPP(), tok->astOperand1(), tok->astOperand2(), _settings->library.functionpure)) {
663+
664+
const bool alwaysTrue(tok->str() == "||");
665+
incorrectConditionError(tok, tok->expressionString(), alwaysTrue);
666+
continue;
667+
}
668+
669+
670+
// 'A && (!A || B)' is equivalent with 'A && B'
671+
// 'A || (!A && B)' is equivalent with 'A || B'
672+
if (printStyle &&
673+
((tok->str() == "||" && tok->astOperand2()->str() == "&&") ||
674+
(tok->str() == "&&" && tok->astOperand2()->str() == "||"))) {
675+
const Token* tok2 = tok->astOperand2()->astOperand1();
676+
if (isIncorrectCond(true, _tokenizer->isCPP(), tok->astOperand1(), tok2, _settings->library.functionpure)) {
677+
std::string expr1(tok->astOperand1()->expressionString());
678+
std::string expr2(tok->astOperand2()->astOperand1()->expressionString());
679+
std::string expr3(tok->astOperand2()->astOperand2()->expressionString());
680+
681+
if (expr1.length() + expr2.length() + expr3.length() > 50U) {
682+
if (expr1[0] == '!' && expr2[0] != '!') {
683+
expr1 = "!A";
684+
expr2 = "A";
685+
} else {
686+
expr1 = "A";
687+
expr2 = "!A";
688+
}
689+
690+
expr3 = "B";
691+
}
692+
693+
const std::string cond1 = expr1 + " " + tok->str() + " (" + expr2 + " " + tok->astOperand2()->str() + " " + expr3 + ")";
694+
const std::string cond2 = expr1 + " " + tok->str() + " " + expr3;
695+
696+
redundantConditionError(tok, tok2->expressionString() + ". '" + cond1 + "' is equivalent to '" + cond2 + "'");
697+
continue;
698+
}
699+
}
700+
701+
// Comparison #1 (LHS)
702+
const Token *comp1 = tok->astOperand1();
703+
if (comp1 && comp1->str() == tok->str())
704+
comp1 = comp1->astOperand2();
705+
706+
// Comparison #2 (RHS)
707+
const Token *comp2 = tok->astOperand2();
708+
709+
// Parse LHS
710+
bool not1;
711+
std::string op1, value1;
712+
const Token *expr1;
713+
if (!parseComparison(comp1, &not1, &op1, &value1, &expr1))
714+
continue;
715+
716+
// Parse RHS
717+
bool not2;
718+
std::string op2, value2;
719+
const Token *expr2;
720+
if (!parseComparison(comp2, &not2, &op2, &value2, &expr2))
721+
continue;
722+
723+
if (isSameExpression(_tokenizer->isCPP(), comp1, comp2, _settings->library.functionpure))
724+
continue; // same expressions => only report that there are same expressions
725+
if (!isSameExpression(_tokenizer->isCPP(), expr1, expr2, _settings->library.functionpure)) {
726+
if ((tok->astOperand1()->isConstOp() && tok->astOperand2()->isConstOp()) &&
727+
isIncorrectCond(true, _tokenizer->isCPP(), tok->astOperand1(), tok->astOperand2(), _settings->library.functionpure)) {
728+
729+
const bool alwaysTrue(tok->str() == "||");
730+
incorrectConditionError(tok, tok->expressionString(), alwaysTrue);
731+
}
732+
continue;
733+
}
734+
735+
const bool isfloat = astIsFloat(expr1, true) || MathLib::isFloat(value1) || astIsFloat(expr2, true) || MathLib::isFloat(value2);
736+
737+
// don't check floating point equality comparisons. that is bad
738+
// and deserves different warnings.
739+
if (isfloat && (op1 == "==" || op1 == "!=" || op2 == "==" || op2 == "!="))
740+
continue;
741+
742+
const double d1 = (isfloat) ? MathLib::toDoubleNumber(value1) : 0;
743+
const double d2 = (isfloat) ? MathLib::toDoubleNumber(value2) : 0;
744+
const MathLib::bigint i1 = (isfloat) ? 0 : MathLib::toLongNumber(value1);
745+
const MathLib::bigint i2 = (isfloat) ? 0 : MathLib::toLongNumber(value2);
746+
const bool useUnsignedInt = (std::numeric_limits<MathLib::bigint>::max()==i1)||(std::numeric_limits<MathLib::bigint>::max()==i2);
747+
const MathLib::biguint u1 = (useUnsignedInt) ? MathLib::toLongNumber(value1) : 0;
748+
const MathLib::biguint u2 = (useUnsignedInt) ? MathLib::toLongNumber(value2) : 0;
749+
// evaluate if expression is always true/false
750+
bool alwaysTrue = true, alwaysFalse = true;
751+
bool firstTrue = true, secondTrue = true;
752+
for (int test = 1; test <= 5; ++test) {
753+
// test:
754+
// 1 => testvalue is less than both value1 and value2
755+
// 2 => testvalue is value1
756+
// 3 => testvalue is between value1 and value2
757+
// 4 => testvalue value2
758+
// 5 => testvalue is larger than both value1 and value2
759+
bool result1, result2;
760+
if (isfloat) {
761+
const double testvalue = getvalue<double>(test, d1, d2);
762+
result1 = checkFloatRelation(op1, testvalue, d1);
763+
result2 = checkFloatRelation(op2, testvalue, d2);
764+
} else if (useUnsignedInt) {
765+
const MathLib::biguint testvalue = getvalue<MathLib::biguint>(test, u1, u2);
766+
result1 = checkIntRelation(op1, testvalue, u1);
767+
result2 = checkIntRelation(op2, testvalue, u2);
768+
} else {
769+
const MathLib::bigint testvalue = getvalue<MathLib::bigint>(test, i1, i2);
770+
result1 = checkIntRelation(op1, testvalue, i1);
771+
result2 = checkIntRelation(op2, testvalue, i2);
772+
}
773+
if (not1)
774+
result1 = !result1;
775+
if (not2)
776+
result2 = !result2;
777+
if (tok->str() == "&&") {
778+
alwaysTrue &= (result1 && result2);
779+
alwaysFalse &= !(result1 && result2);
780+
} else {
781+
alwaysTrue &= (result1 || result2);
782+
alwaysFalse &= !(result1 || result2);
783+
}
784+
firstTrue &= !(!result1 && result2);
785+
secondTrue &= !(result1 && !result2);
786+
}
787+
788+
const std::string cond1str = conditionString(not1, expr1, op1, value1);
789+
const std::string cond2str = conditionString(not2, expr2, op2, value2);
790+
if (printWarning && (alwaysTrue || alwaysFalse)) {
791+
const std::string text = cond1str + " " + tok->str() + " " + cond2str;
792+
incorrectConditionError(tok, text, alwaysTrue);
793+
} else if (printStyle && secondTrue) {
794+
const std::string text = "If '" + cond1str + "', the comparison '" + cond2str +
795+
"' is always " + (secondTrue ? "true" : "false") + ".";
796+
redundantConditionError(tok, text);
797+
} else if (printStyle && firstTrue) {
798+
//const std::string text = "The comparison " + cond1str + " is always " +
799+
// (firstTrue ? "true" : "false") + " when " +
800+
// cond2str + ".";
801+
const std::string text = "If '" + cond2str + "', the comparison '" + cond1str +
802+
"' is always " + (firstTrue ? "true" : "false") + ".";
803+
redundantConditionError(tok, text);
804+
}
805+
}
806+
}
807+
}
808+
809+
void CheckCondition::incorrectConditionError(const Token *tok, const std::string &condition, bool always)
810+
{
811+
if (always)
812+
reportError(tok, Severity::warning, "IncorrectCondition",
813+
"Condition always evaluates to true: " + condition + ".\n"
814+
"Condition always evaluates to true: " + condition + ". "
815+
"Are these conditions necessary? Did you intend to use && instead? Are the numbers correct? Are you comparing the correct variables?");
816+
else
817+
reportError(tok, Severity::warning, "IncorrectCondition",
818+
"Condition always evaluates to false: " + condition + ".\n"
819+
"Condition always evaluates to false: " + condition + ". "
820+
"Are these conditions necessary? Did you intend to use || instead? Are the numbers correct? Are you comparing the correct variables?");
821+
}
822+
823+
824+
644825
void CheckCondition::checkIncorrectLogicOperator()
645826
{
646827
const bool printStyle = _settings->isEnabled("style");
@@ -813,6 +994,7 @@ void CheckCondition::incorrectLogicOperatorError(const Token *tok, const std::st
813994
"Are these conditions necessary? Did you intend to use || instead? Are the numbers correct? Are you comparing the correct variables?");
814995
}
815996

997+
816998
void CheckCondition::redundantConditionError(const Token *tok, const std::string &text)
817999
{
8181000
reportError(tok, Severity::style, "redundantCondition", "Redundant condition: " + text);

‎lib/checkcondition.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ class CPPCHECKLIB CheckCondition : public Check {
5050
checkCondition.clarifyCondition(); // not simplified because ifAssign
5151
checkCondition.oppositeInnerCondition();
5252
checkCondition.checkIncorrectLogicOperator();
53+
checkCondition.checkIncorrectCondition();
5354
checkCondition.checkInvalidTestForOverflow();
5455
}
5556

@@ -88,6 +89,7 @@ class CPPCHECKLIB CheckCondition : public Check {
8889

8990
/** @brief %Check for testing for mutual exclusion over ||*/
9091
void checkIncorrectLogicOperator();
92+
void checkIncorrectCondition();
9193

9294
/** @brief %Check for suspicious usage of modulo (e.g. "if(var % 4 == 4)") */
9395
void checkModuloAlwaysTrueFalse();
@@ -118,6 +120,7 @@ class CPPCHECKLIB CheckCondition : public Check {
118120
void oppositeInnerConditionError(const Token *tok1, const Token* tok2);
119121

120122
void incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always);
123+
void incorrectConditionError(const Token *tok, const std::string &condition, bool always);
121124
void redundantConditionError(const Token *tok, const std::string &text);
122125

123126
void moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal);

0 commit comments

Comments
 (0)