Fix #15031 (False positive: knownConditionTrueFalse reported when calling a function with a known result) (#8853)

Author: danmar

.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

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;

man/checkers/knownConditionTrueFalse.md

@@ -1,62 +1,66 @@
 # knownConditionTrueFalse
 
 **Message**: Condition 'x==5' is always true<br/>
-**Category**: Correctness<br/>
+**Category**: Code cleanup<br/>
 **Severity**: Style<br/>
 **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 <map>
-  #include <string>
-  struct S { int i; };
-  struct T {
-      std::map<std::string, S*> 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
 

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"