diff --git a/.selfcheck_suppressions b/.selfcheck_suppressions index 63d33fd2704..863702d6c60 100644 --- a/.selfcheck_suppressions +++ b/.selfcheck_suppressions @@ -76,7 +76,6 @@ funcArgNamesDifferent:externals/tinyxml2/tinyxml2.cpp funcArgNamesDifferentUnnamed:externals/tinyxml2/tinyxml2.cpp funcArgNamesDifferentUnnamed:externals/tinyxml2/tinyxml2.h nullPointerRedundantCheck:externals/tinyxml2/tinyxml2.cpp -knownConditionTrueFalse:externals/tinyxml2/tinyxml2.cpp useStlAlgorithm:externals/simplecpp/simplecpp.cpp funcArgNamesDifferentUnnamed:externals/simplecpp/simplecpp.h missingMemberCopy:externals/simplecpp/simplecpp.h diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index e47e3961258..be7ce2d0232 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -1556,7 +1556,7 @@ void CheckConditionImpl::alwaysTrueFalse() condition = parent->astParent()->astParent()->previous(); else if (Token::Match(tok, "%comp%")) condition = tok; - else if ((tok->str() == "(" || (hasComp && Token::Match(tok, "!|%var%"))) && astIsBool(parent) && Token::Match(parent, "%assign%")) + else if (hasComp && Token::Match(tok, "!|%var%") && astIsBool(parent) && Token::Match(parent, "%assign%")) condition = tok; else continue; diff --git a/man/checkers/knownConditionTrueFalse.md b/man/checkers/knownConditionTrueFalse.md index 1e354d9827c..51b652c7b90 100644 --- a/man/checkers/knownConditionTrueFalse.md +++ b/man/checkers/knownConditionTrueFalse.md @@ -1,62 +1,66 @@ # knownConditionTrueFalse **Message**: Condition 'x==5' is always true
-**Category**: Correctness
+**Category**: Code cleanup
**Severity**: Style
**Language**: C/C++ ## Description -cppcheck can already work out this condition's value from what it knows about the variables involved, -so the condition is always true or always false. +A condition is always true or always false. -## Motivation +Note: a warning is not written for obvious cases like `if (false)`. + +If a condition is always true then technically the condition is redundant. It +can be removed so that the conditional code will be unconditionally executed. +This reduces complexity. + +If a condition is always false then the conditional code is unreachable and can +be removed. + +It is however also possible that the intended check never actually happens and +a real bug (a wrong comparison, a typo'd variable, a value that was supposed to +vary but doesn't) slips through unnoticed. -A condition that's always true or always false isn't testing anything - at best it's confusing, -leftover, or dead code; at worst, it means the intended check never actually happens and a real bug (a -wrong comparison, a typo'd variable, a value that was supposed to vary but doesn't) slips through -unnoticed. +## Motivation -This check may need `--check-level=exhaustive` to see every case. +The condition may be invariant (always true or always false) by mistake, +otherwise it is possible to cleanup redundant code to reduce complexity. ## How to fix -Before: +Before (condition is always true): ```cpp void f() { int x = 5; - if (x == 5) {} // <- always true + if (x == 5) { // <- always true + dostuff(); + } +} +``` + +After: The condition is technically redundant, this code is logically the same. +```cpp +void f() { + dostuff(); } ``` -After: use the real variable instead of a fixed value, or remove the redundant check. +Before (condition is always false): ```cpp -void f(int x) { - if (x == 5) {} +void f() { + int x = 5; + if (x < 3) { // <- always false + dostuff(); + } } ``` -## False positives to be aware of - -- **This check does not account for a member value changing through a call that reaches it indirectly** - (for example, through a container of pointers the function iterates over). A member read before such - a call can be wrongly assumed to still hold the same value afterwards: - ```cpp - #include - #include - struct S { int i; }; - struct T { - std::map m; - S* get(const std::string& s) { return m[s]; } - void modify() { for (const auto& e : m) e.second->i = 0; } - }; - void f(T& t) { - const S* p = t.get("abc"); - const int o = p->i; - t.modify(); // this can change p->i - if (p->i == o) {} // wrongly reported as always true - } - ``` +After: The conditional code is unreachable, this code is logically the same. +```cpp +void f() { +} +``` ## Related checkers diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 5d57ad3e312..5e35ef08f6b 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -3668,7 +3668,7 @@ class TestCondition : public TestFixture { "}\n"); ASSERT_EQUALS("", errout_str()); - check("long X::g(bool unknown, int& result) {\n" + check("long g(bool unknown, int& result) {\n" " long ret = 0;\n" " bool f = false;\n" " f = f || unknown;\n" @@ -4878,7 +4878,18 @@ class TestCondition : public TestFixture { " }\n" " return false;\n" "}\n"); - ASSERT_EQUALS("[test.cpp:6:12] -> [test.cpp:7:21]: (style) Assigned value 's.g()' is always true [knownConditionTrueFalse]\n", errout_str()); + TODO_ASSERT_EQUALS("[test.cpp:6:12] -> [test.cpp:7:21]: (style) Assigned value 's.g()' is always true [knownConditionTrueFalse]\n", "", errout_str()); + + check("static bool parse(int r) {\n" // #15031 + " bool res = false;\n" + " return res;\n" + "}\n" + "\n" + "int main (void) {\n" + " bool res = parse(1101);\n" + " return res;\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); check("void f(const void* p) {\n" // #11519 " bool b = false;\n"