From a7b82b5c2812f0f5df8978525ac161845940baae Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Sat, 13 Jun 2015 18:08:13 +0200 Subject: [PATCH] Refactoring to address some issues from #5895 (handling of unsigned numbers). Also adding a TODO testcase since the real issue (FP) is still not fixed --- Makefile | 4 ++++ lib/checkcondition.cpp | 36 +++++++++++++++++++++++++++--------- lib/tokenlist.cpp | 2 +- test/testcondition.cpp | 10 +++++++++- test/testfiles.pri | 1 + 5 files changed, 42 insertions(+), 11 deletions(-) diff --git a/Makefile b/Makefile index 46c8d9851..69282bfa7 100644 --- a/Makefile +++ b/Makefile @@ -214,6 +214,7 @@ TESTOBJ = test/options.o \ test/testtimer.o \ test/testtoken.o \ test/testtokenize.o \ + test/testtokenlist.o \ test/testtype.o \ test/testuninitvar.o \ test/testunusedfunctions.o \ @@ -573,6 +574,9 @@ test/testtoken.o: test/testtoken.cpp lib/cxx11emu.h test/testsuite.h lib/errorlo test/testtokenize.o: test/testtokenize.cpp lib/cxx11emu.h test/testsuite.h lib/errorlogger.h lib/config.h lib/suppressions.h test/redirect.h lib/library.h lib/mathlib.h lib/token.h lib/valueflow.h lib/tokenize.h lib/tokenlist.h lib/settings.h lib/standards.h lib/timer.h lib/path.h lib/preprocessor.h $(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CFG) $(CXXFLAGS) $(UNDEF_STRICT_ANSI) -std=c++0x -c -o test/testtokenize.o test/testtokenize.cpp +test/testtokenlist.o: test/testtokenlist.cpp lib/cxx11emu.h test/testsuite.h lib/errorlogger.h lib/config.h lib/suppressions.h test/redirect.h lib/library.h lib/mathlib.h lib/token.h lib/valueflow.h lib/settings.h lib/standards.h lib/timer.h lib/tokenlist.h + $(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CFG) $(CXXFLAGS) $(UNDEF_STRICT_ANSI) -std=c++0x -c -o test/testtokenlist.o test/testtokenlist.cpp + test/testtype.o: test/testtype.cpp lib/cxx11emu.h lib/preprocessor.h lib/config.h lib/tokenize.h lib/errorlogger.h lib/suppressions.h lib/tokenlist.h lib/symboldatabase.h lib/token.h lib/valueflow.h lib/mathlib.h lib/checktype.h lib/check.h lib/settings.h lib/library.h lib/standards.h lib/timer.h test/testsuite.h test/redirect.h test/testutils.h $(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CFG) $(CXXFLAGS) $(UNDEF_STRICT_ANSI) -std=c++0x -c -o test/testtype.o test/testtype.cpp diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index e6b371e8c..3772505c8 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -525,7 +525,8 @@ static std::string invertOperatorForOperandSwap(std::string s) return s; } -static bool checkIntRelation(const std::string &op, const MathLib::bigint value1, const MathLib::bigint value2) +template +static bool checkIntRelation(const std::string &op, const T value1, const T value2) { return (op == "==" && value1 == value2) || (op == "!=" && value1 != value2) || @@ -543,7 +544,9 @@ static bool checkFloatRelation(const std::string &op, const double value1, const (op == "<=" && value1 <= value2); } -template static T getvalue(const int test, const T value1, const T value2) + +template +static inline T getvalue(const int test, const T value1, const T value2) { // test: // 1 => return value that is less than both value1 and value2 @@ -553,7 +556,7 @@ template static T getvalue(const int test, const T value1, const T valu // 5 => return value that is larger than both value1 and value2 switch (test) { case 1: { - T ret = std::min(value1, value2); + const T ret = std::min(value1, value2); if ((ret - (T)1) < ret) return ret - (T)1; else if ((ret / (T)2) < ret) @@ -565,11 +568,19 @@ template static T getvalue(const int test, const T value1, const T valu case 2: return value1; case 3: - return (value1 + value2) / (T)2; + if (std::numeric_limits::is_integer) { + const T min = std::min(value1, value2); + if (min== std::numeric_limits::max()) + return min; + else + return min+1; // see #5895 + } else { + return (value1 + value2) / (T)2; + } case 4: return value2; case 5: { - T ret = std::max(value1, value2); + const T ret = std::max(value1, value2); if ((ret + (T)1) > ret) return ret + (T)1; else if ((ret / (T)2) > ret) @@ -672,6 +683,13 @@ void CheckCondition::checkIncorrectLogicOperator() if (isfloat && (op1 == "==" || op1 == "!=" || op2 == "==" || op2 == "!=")) continue; + const double d1 = (isfloat) ? MathLib::toDoubleNumber(value1) : 0; + const double d2 = (isfloat) ? MathLib::toDoubleNumber(value2) : 0; + const MathLib::bigint i1 = (isfloat) ? 0 : MathLib::toLongNumber(value1); + const MathLib::bigint i2 = (isfloat) ? 0 : MathLib::toLongNumber(value2); + const bool useUnsignedInt = (std::numeric_limits::max()==i1)||(std::numeric_limits::max()==i2); + const MathLib::biguint u1 = (useUnsignedInt) ? MathLib::toLongNumber(value1) : 0; + const MathLib::biguint u2 = (useUnsignedInt) ? MathLib::toLongNumber(value2) : 0; // evaluate if expression is always true/false bool alwaysTrue = true, alwaysFalse = true; bool firstTrue = true, secondTrue = true; @@ -684,14 +702,14 @@ void CheckCondition::checkIncorrectLogicOperator() // 5 => testvalue is larger than both value1 and value2 bool result1, result2; if (isfloat) { - const double d1 = MathLib::toDoubleNumber(value1); - const double d2 = MathLib::toDoubleNumber(value2); const double testvalue = getvalue(test, d1, d2); result1 = checkFloatRelation(op1, testvalue, d1); result2 = checkFloatRelation(op2, testvalue, d2); + } else if (useUnsignedInt) { + const MathLib::biguint testvalue = getvalue(test, u1, u2); + result1 = checkIntRelation(op1, testvalue, u1); + result2 = checkIntRelation(op2, testvalue, u2); } else { - const MathLib::bigint i1 = MathLib::toLongNumber(value1); - const MathLib::bigint i2 = MathLib::toLongNumber(value2); const MathLib::bigint testvalue = getvalue(test, i1, i2); result1 = checkIntRelation(op1, testvalue, i1); result2 = checkIntRelation(op2, testvalue, i2); diff --git a/lib/tokenlist.cpp b/lib/tokenlist.cpp index d55d450b3..deac18545 100644 --- a/lib/tokenlist.cpp +++ b/lib/tokenlist.cpp @@ -132,7 +132,7 @@ void TokenList::addtoken(const std::string & str, const unsigned int lineno, con std::string str2; if (MathLib::isHex(str) || MathLib::isOct(str) || MathLib::isBin(str)) { std::ostringstream str2stream; - str2stream << MathLib::toLongNumber(str); + str2stream << MathLib::MathLib::toULongNumber(str); str2 = str2stream.str(); } else if (str.compare(0, 5, "_Bool") == 0) { str2 = "bool"; diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 81fb0845a..d5752ac1d 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -48,6 +48,7 @@ private: TEST_CASE(incorrectLogicOperator7); // opposite expressions: (expr || !expr) TEST_CASE(secondAlwaysTrueFalseWhenFirstTrueError); TEST_CASE(incorrectLogicOp_condSwapping); + TEST_CASE(testBug5895); TEST_CASE(modulo); @@ -656,7 +657,6 @@ private: " if (x == 1.0 && x == 3.0)\n" " a++;\n" "}"); - //ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1.0 && x == 3.0.\n", errout.str()); ASSERT_EQUALS("", errout.str()); // float comparisons with == and != are not checked right now - such comparison is a bad idea check("void f(float x) {\n" @@ -1408,6 +1408,14 @@ private: "}"); ASSERT_EQUALS("", errout.str()); } + + void testBug5895() { + check("void png_parse(uint64_t init, int buf_size) {\n" + " if (init == 0x89504e470d0a1a0a || init == 0x8a4d4e470d0a1a0a)\n" + " ;\n" + "}"); + TODO_ASSERT_EQUALS("", "[test.cpp:2]: (style) Redundant condition: If init == 9894494448401390090, the comparison init == 9965707617509186058 is always true.\n", errout.str()); + } }; REGISTER_TEST(TestCondition) diff --git a/test/testfiles.pri b/test/testfiles.pri index a6cdf2161..a748eddda 100644 --- a/test/testfiles.pri +++ b/test/testfiles.pri @@ -50,6 +50,7 @@ SOURCES += $${BASEPATH}/test64bit.cpp \ $${BASEPATH}/testtimer.cpp \ $${BASEPATH}/testtoken.cpp \ $${BASEPATH}/testtokenize.cpp \ + $${BASEPATH}/testtokenlist.cpp \ $${BASEPATH}/testtype.cpp \ $${BASEPATH}/testuninitvar.cpp \ $${BASEPATH}/testunusedfunctions.cpp \