Enable and mitigate readability-simplify-boolean-expr (danmar#4897)

chrchr-github · web-flow · commit 3ccd0505cdc9 · 2023-03-17T13:51:55.000+01:00
diff --git a/.clang-tidy b/.clang-tidy
@@ -1,7 +1,9 @@
 ---
-Checks:          '*,-abseil-*,-altera-*,-android-*,-boost-*,-cert-*,-cppcoreguidelines-*,-darwin-*,-fuchsia-*,-google-*,-hicpp-*,-linuxkernel-*,-llvm-*,-llvmlibc-*,-mpi-*,-objc-*,-openmp-*,-zircon-*,google-explicit-constructor,-readability-braces-around-statements,-readability-magic-numbers,-bugprone-macro-parentheses,-readability-isolate-declaration,-readability-function-size,-modernize-use-trailing-return-type,-readability-implicit-bool-conversion,-readability-uppercase-literal-suffix,-modernize-use-auto,-readability-else-after-return,-modernize-use-default-member-init,-readability-redundant-member-init,-modernize-avoid-c-arrays,-modernize-use-equals-default,-readability-container-size-empty,-readability-simplify-boolean-expr,-bugprone-branch-clone,-bugprone-narrowing-conversions,-modernize-raw-string-literal,-readability-convert-member-functions-to-static,-modernize-loop-convert,-readability-const-return-type,-modernize-return-braced-init-list,-performance-inefficient-string-concatenation,-misc-throw-by-value-catch-by-reference,-readability-avoid-const-params-in-decls,-misc-non-private-member-variables-in-classes,-clang-analyzer-*,-bugprone-signed-char-misuse,-misc-no-recursion,-readability-use-anyofallof,-performance-no-automatic-move,-readability-function-cognitive-complexity,-readability-redundant-access-specifiers,-concurrency-mt-unsafe,-bugprone-easily-swappable-parameters,-readability-suspicious-call-argument,-readability-identifier-length,-readability-container-data-pointer,-bugprone-assignment-in-if-condition,-misc-const-correctness,-portability-std-allocator-const,-modernize-deprecated-ios-base-aliases,-bugprone-unchecked-optional-access,-modernize-replace-auto-ptr,-readability-identifier-naming,-portability-simd-intrinsics,-misc-use-anonymous-namespace'
+Checks:          '*,-abseil-*,-altera-*,-android-*,-boost-*,-cert-*,-cppcoreguidelines-*,-darwin-*,-fuchsia-*,-google-*,-hicpp-*,-linuxkernel-*,-llvm-*,-llvmlibc-*,-mpi-*,-objc-*,-openmp-*,-zircon-*,google-explicit-constructor,-readability-braces-around-statements,-readability-magic-numbers,-bugprone-macro-parentheses,-readability-isolate-declaration,-readability-function-size,-modernize-use-trailing-return-type,-readability-implicit-bool-conversion,-readability-uppercase-literal-suffix,-modernize-use-auto,-readability-else-after-return,-modernize-use-default-member-init,-readability-redundant-member-init,-modernize-avoid-c-arrays,-modernize-use-equals-default,-readability-container-size-empty,-bugprone-branch-clone,-bugprone-narrowing-conversions,-modernize-raw-string-literal,-readability-convert-member-functions-to-static,-modernize-loop-convert,-readability-const-return-type,-modernize-return-braced-init-list,-performance-inefficient-string-concatenation,-misc-throw-by-value-catch-by-reference,-readability-avoid-const-params-in-decls,-misc-non-private-member-variables-in-classes,-clang-analyzer-*,-bugprone-signed-char-misuse,-misc-no-recursion,-readability-use-anyofallof,-performance-no-automatic-move,-readability-function-cognitive-complexity,-readability-redundant-access-specifiers,-concurrency-mt-unsafe,-bugprone-easily-swappable-parameters,-readability-suspicious-call-argument,-readability-identifier-length,-readability-container-data-pointer,-bugprone-assignment-in-if-condition,-misc-const-correctness,-portability-std-allocator-const,-modernize-deprecated-ios-base-aliases,-bugprone-unchecked-optional-access,-modernize-replace-auto-ptr,-readability-identifier-naming,-portability-simd-intrinsics,-misc-use-anonymous-namespace'
 WarningsAsErrors: '*'
 HeaderFilterRegex: '(cli|gui|lib|oss-fuzz|test|triage)\/[a-z]+\.h'
 CheckOptions:
   - key:             misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic
-    value:           '1'
+    value:           '1'
+  - key:             readability-simplify-boolean-expr.SimplifyDeMorgan
+    value:           '0'
diff --git a/clang-tidy.md b/clang-tidy.md
@@ -49,7 +49,6 @@ We are not interesting in the size/complexity of a function.
 
 `readability-magic-numbers`<br>
 `readability-redundant-member-init`<br>
-`readability-simplify-boolean-expr`<br>
 
 These do not (always) increase readability.
 
diff --git a/gui/cppchecklibrarydata.cpp b/gui/cppchecklibrarydata.cpp
@@ -365,12 +365,12 @@ static CppcheckLibraryData::Markup loadMarkup(QXmlStreamReader &xmlReader)
         mandatoryAttibuteMissing(xmlReader, "ext");
     }
     if (xmlReader.attributes().hasAttribute("aftercode")) {
-        markup.afterCode = (xmlReader.attributes().value("aftercode") == QString("true")) ? true : false;
+        markup.afterCode = (xmlReader.attributes().value("aftercode") == QString("true"));
     } else {
         mandatoryAttibuteMissing(xmlReader, "aftercode");
     }
     if (xmlReader.attributes().hasAttribute("reporterrors")) {
-        markup.reportErrors = (xmlReader.attributes().value("reporterrors") == QString("true")) ? true : false;
+        markup.reportErrors = (xmlReader.attributes().value("reporterrors") == QString("true"));
     } else {
         mandatoryAttibuteMissing(xmlReader, "reporterrors");
     }
diff --git a/gui/settingsdialog.cpp b/gui/settingsdialog.cpp
@@ -147,10 +147,7 @@ Qt::CheckState SettingsDialog::boolToCheckState(bool yes)
 
 bool SettingsDialog::checkStateToBool(Qt::CheckState state)
 {
-    if (state == Qt::Checked) {
-        return true;
-    }
-    return false;
+    return state == Qt::Checked;
 }
 
 
diff --git a/lib/astutils.cpp b/lib/astutils.cpp
@@ -392,9 +392,7 @@ bool isVariableDecl(const Token* tok)
     if (var->nameToken() == tok)
         return true;
     const Token * const varDeclEndToken = var->declEndToken();
-    if (Token::Match(varDeclEndToken, "; %var%") && varDeclEndToken->next() == tok)
-        return true;
-    return false;
+    return Token::Match(varDeclEndToken, "; %var%") && varDeclEndToken->next() == tok;
 }
 
 bool isStlStringType(const Token* tok)
@@ -2002,11 +2000,7 @@ bool isUniqueExpression(const Token* tok)
                 return true;
 
             const std::string freturnType = f.retType ? f.retType->name() : f.retDef->stringifyList(f.returnDefEnd());
-            if (f.argumentList.size() == fun->argumentList.size() && returnType == freturnType &&
-                f.name() != fun->name()) {
-                return false;
-            }
-            return true;
+            return f.argumentList.size() != fun->argumentList.size() || returnType != freturnType || f.name() == fun->name();
         }))
             return false;
     } else if (tok->variable()) {
diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp
@@ -1111,12 +1111,10 @@ static bool isVLAIndex(const Token* tok)
         return true;
     if (tok->str() == "?") {
         // this is a VLA index if both expressions around the ":" is VLA index
-        if (tok->astOperand2() &&
-            tok->astOperand2()->str() == ":" &&
-            isVLAIndex(tok->astOperand2()->astOperand1()) &&
-            isVLAIndex(tok->astOperand2()->astOperand2()))
-            return true;
-        return false;
+        return tok->astOperand2() &&
+               tok->astOperand2()->str() == ":" &&
+               isVLAIndex(tok->astOperand2()->astOperand1()) &&
+               isVLAIndex(tok->astOperand2()->astOperand2());
     }
     return isVLAIndex(tok->astOperand1()) || isVLAIndex(tok->astOperand2());
 }
diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp
@@ -2894,7 +2894,7 @@ void CheckClass::checkCopyCtorAndEqOperator()
     // This is disabled because of #8388
     // The message must be clarified. How is the behaviour different?
     // cppcheck-suppress unreachableCode - remove when code is enabled again
-    if ((true) || !mSettings->severity.isEnabled(Severity::warning))
+    if ((true) || !mSettings->severity.isEnabled(Severity::warning)) // NOLINT(readability-simplify-boolean-expr)
         return;
 
     for (const Scope * scope : mSymbolDatabase->classAndStructScopes) {
diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp
@@ -364,15 +364,15 @@ void CheckCondition::comparison()
                 if ((expr1->str() == "&" && (num1 & num2) != num2) ||
                     (expr1->str() == "|" && (num1 | num2) != num2)) {
                     const std::string& op(tok->str());
-                    comparisonError(expr1, expr1->str(), num1, op, num2, op=="==" ? false : true);
+                    comparisonError(expr1, expr1->str(), num1, op, num2, op != "==");
                 }
             } else if (expr1->str() == "&") {
                 const bool or_equal = Token::Match(tok, ">=|<=");
                 const std::string& op(tok->str());
                 if ((Token::Match(tok, ">=|<")) && (num1 < num2)) {
-                    comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? false : true);
+                    comparisonError(expr1, expr1->str(), num1, op, num2, !or_equal);
                 } else if ((Token::Match(tok, "<=|>")) && (num1 <= num2)) {
-                    comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? true : false);
+                    comparisonError(expr1, expr1->str(), num1, op, num2, or_equal);
                 }
             } else if (expr1->str() == "|") {
                 if ((expr1->astOperand1()->valueType()) &&
@@ -382,11 +382,11 @@ void CheckCondition::comparison()
                     if ((Token::Match(tok, ">=|<")) && (num1 >= num2)) {
                         //"(a | 0x07) >= 7U" is always true for unsigned a
                         //"(a | 0x07) < 7U" is always false for unsigned a
-                        comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? true : false);
+                        comparisonError(expr1, expr1->str(), num1, op, num2, or_equal);
                     } else if ((Token::Match(tok, "<=|>")) && (num1 > num2)) {
                         //"(a | 0x08) <= 7U" is always false for unsigned a
                         //"(a | 0x07) > 6U" is always true for unsigned a
-                        comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? false : true);
+                        comparisonError(expr1, expr1->str(), num1, op, num2, !or_equal);
                     }
                 }
             }
@@ -1048,10 +1048,7 @@ static bool parseComparison(const Token *comp, bool &not1, std::string &op, std:
     inconclusive = inconclusive || ((value)[0] == '\'' && !(op == "!=" || op == "=="));
 
     // Only float and int values are currently handled
-    if (!MathLib::isInt(value) && !MathLib::isFloat(value) && (value)[0] != '\'')
-        return false;
-
-    return true;
+    return MathLib::isInt(value) || MathLib::isFloat(value) || (value[0] == '\'');
 }
 
 static std::string conditionString(bool not1, const Token *expr1, const std::string &op, const std::string &value1)
diff --git a/lib/checkother.cpp b/lib/checkother.cpp
@@ -554,10 +554,7 @@ void CheckOther::redundantAssignmentInSwitchError(const Token *tok1, const Token
 //---------------------------------------------------------------------------
 static inline bool isFunctionOrBreakPattern(const Token *tok)
 {
-    if (Token::Match(tok, "%name% (") || Token::Match(tok, "break|continue|return|exit|goto|throw"))
-        return true;
-
-    return false;
+    return Token::Match(tok, "%name% (") || Token::Match(tok, "break|continue|return|exit|goto|throw");
 }
 
 void CheckOther::checkRedundantAssignmentInSwitch()
@@ -1097,7 +1094,7 @@ void CheckOther::variableScopeError(const Token *tok, const std::string &varname
 void CheckOther::checkCommaSeparatedReturn()
 {
     // This is experimental for now. See #5076
-    if ((true) || !mSettings->severity.isEnabled(Severity::style))
+    if ((true) || !mSettings->severity.isEnabled(Severity::style)) // NOLINT(readability-simplify-boolean-expr)
         return;
 
     for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) {
diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp
@@ -702,9 +702,7 @@ static bool isSameIteratorContainerExpression(const Token* tok1,
                                               ValueFlow::Value::LifetimeKind kind = ValueFlow::Value::LifetimeKind::Iterator)
 {
     if (isSameExpression(true, false, tok1, tok2, library, false, false)) {
-        if (astIsContainerOwned(tok1) && isTemporary(true, tok1, &library))
-            return false;
-        return true;
+        return !astIsContainerOwned(tok1) || !isTemporary(true, tok1, &library);
     }
     if (kind == ValueFlow::Value::LifetimeKind::Address) {
         return isSameExpression(true, false, getAddressContainer(tok1), getAddressContainer(tok2), library, false, false);
diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp
@@ -1469,9 +1469,7 @@ bool CheckUninitVar::isMemberVariableUsage(const Token *tok, bool isPointer, All
 
     if (Token::Match(tok, "%name% . %name%") && tok->strAt(2) == membervar && !(tok->tokAt(-2)->variable() && tok->tokAt(-2)->variable()->isReference())) {
         const Token *parent = tok->next()->astParent();
-        if (parent && parent->isUnaryOp("&"))
-            return false;
-        return true;
+        return !parent || !parent->isUnaryOp("&");
     } else if (!isPointer && !Token::simpleMatch(tok->astParent(), ".") && Token::Match(tok->previous(), "[(,] %name% [,)]") && isVariableUsage(tok, isPointer, alloc))
         return true;
 
@@ -1487,7 +1485,7 @@ bool CheckUninitVar::isMemberVariableUsage(const Token *tok, bool isPointer, All
         return true;
 
     // TODO: this used to be experimental - enable or remove see #5586
-    else if ((false) &&
+    else if ((false) && // NOLINT(readability-simplify-boolean-expr)
              !isPointer &&
              Token::Match(tok->tokAt(-2), "[(,] & %name% [,)]") &&
              isVariableUsage(tok, isPointer, alloc))
diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp
@@ -825,9 +825,7 @@ struct ForwardTraversal {
     }
 
     static bool isUnevaluated(const Token* tok) {
-        if (Token::Match(tok->previous(), "sizeof|decltype ("))
-            return true;
-        return false;
+        return Token::Match(tok->previous(), "sizeof|decltype (");
     }
 
     static bool isFunctionCall(const Token* tok)
diff --git a/lib/path.cpp b/lib/path.cpp
@@ -140,14 +140,10 @@ bool Path::isAbsolute(const std::string& path)
         return false;
 
     // On Windows, 'C:\foo\bar' is an absolute path, while 'C:foo\bar' is not
-    if (nativePath.compare(0, 2, "\\\\") == 0 || (std::isalpha(nativePath[0]) != 0 && nativePath.compare(1, 2, ":\\") == 0))
-        return true;
+    return nativePath.compare(0, 2, "\\\\") == 0 || (std::isalpha(nativePath[0]) != 0 && nativePath.compare(1, 2, ":\\") == 0);
 #else
-    if (!nativePath.empty() && nativePath[0] == '/')
-        return true;
+    return !nativePath.empty() && nativePath[0] == '/';
 #endif
-
-    return false;
 }
 
 std::string Path::getRelativePath(const std::string& absolutePath, const std::vector<std::string>& basePaths)
diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp
@@ -1754,7 +1754,7 @@ bool SymbolDatabase::isFunction(const Token *tok, const Scope* outerScope, const
     // function returning function pointer? '... ( ... %name% ( ... ))( ... ) {'
     // function returning reference to array '... ( & %name% ( ... ))[ ... ] {'
     // TODO: Activate this again
-    if ((false) && tok->str() == "(" && tok->strAt(1) != "*" &&
+    if ((false) && tok->str() == "(" && tok->strAt(1) != "*" && // NOLINT(readability-simplify-boolean-expr)
         (tok->link()->previous()->str() == ")" || Token::simpleMatch(tok->link()->tokAt(-2), ") const"))) {
         const Token* tok2 = tok->link()->next();
         if (tok2 && tok2->str() == "(" && Token::Match(tok2->link()->next(), "{|;|const|=")) {
diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h
@@ -1094,9 +1094,7 @@ class CPPCHECKLIB Scope {
         const Scope * parent = nestedIn;
         while (outer != parent && parent)
             parent = parent->nestedIn;
-        if (parent && parent == outer)
-            return true;
-        return false;
+        return parent && parent == outer;
     }
 
     static Function* nestedInFunction(const Scope* scope) {
diff --git a/lib/templatesimplifier.cpp b/lib/templatesimplifier.cpp
@@ -1877,10 +1877,7 @@ void TemplateSimplifier::expandTemplate(
             if (tok3->next() && tok3->next()->str()=="<") {
                 std::vector<const Token *> localTypeParametersInDeclaration;
                 getTemplateParametersInDeclaration(tok3->tokAt(2), localTypeParametersInDeclaration);
-                if (localTypeParametersInDeclaration.size() != typeParametersInDeclaration.size())
-                    inTemplateDefinition = false; // Partial specialization
-                else
-                    inTemplateDefinition = true;
+                inTemplateDefinition = localTypeParametersInDeclaration.size() == typeParametersInDeclaration.size(); // Partial specialization
             } else {
                 inTemplateDefinition = false; // Only template instantiation
             }
diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp
@@ -325,17 +325,9 @@ bool Tokenizer::duplicateTypedef(Token **tokPtr, const Token *name, const Token
                                 return false;
                             }
                         } else if (tok->previous()->str() == "union") {
-                            if (tok->next()->str() != ";") {
-                                return true;
-                            } else {
-                                return false;
-                            }
+                            return tok->next()->str() != ";";
                         } else if (isCPP() && tok->previous()->str() == "class") {
-                            if (tok->next()->str() != ";") {
-                                return true;
-                            } else {
-                                return false;
-                            }
+                            return tok->next()->str() != ";";
                         }
                         if (tok)
                             tok = tok->previous();
@@ -7367,7 +7359,7 @@ void Tokenizer::validate() const
             if (tok->link() == nullptr)
                 cppcheckError(tok);
 
-            if (linkTokens.empty() == true)
+            if (linkTokens.empty())
                 cppcheckError(tok);
 
             if (tok->link() != linkTokens.top())
diff --git a/lib/tokenlist.cpp b/lib/tokenlist.cpp
@@ -621,9 +621,7 @@ static bool isQualifier(const Token* tok)
 {
     while (Token::Match(tok, "&|&&|*"))
         tok = tok->next();
-    if (!Token::Match(tok, "{|;"))
-        return false;
-    return true;
+    return Token::Match(tok, "{|;");
 }
 
 static void compileUnaryOp(Token *&tok, AST_state& state, void (*f)(Token *&tok, AST_state& state))
diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp
@@ -8037,9 +8037,7 @@ bool ValueFlow::isContainerSizeChanged(const Token* tok, int indirect, const Set
     case Library::Container::Action::CHANGE_INTERNAL:
         break;
     }
-    if (isContainerSizeChangedByFunction(tok, indirect, settings, depth))
-        return true;
-    return false;
+    return isContainerSizeChangedByFunction(tok, indirect, settings, depth);
 }
 
 static bool isContainerSizeChanged(nonneg int varId,

Original file line number	Diff line number	Diff line change
`@@ -365,12 +365,12 @@ static CppcheckLibraryData::Markup loadMarkup(QXmlStreamReader &xmlReader)`
`365`	`365`	`mandatoryAttibuteMissing(xmlReader, "ext");`
`366`	`366`	`}`
`367`	`367`	`if (xmlReader.attributes().hasAttribute("aftercode")) {`
`368`		`- markup.afterCode = (xmlReader.attributes().value("aftercode") == QString("true")) ? true : false;`
	`368`	`+ markup.afterCode = (xmlReader.attributes().value("aftercode") == QString("true"));`
`369`	`369`	`} else {`
`370`	`370`	`mandatoryAttibuteMissing(xmlReader, "aftercode");`
`371`	`371`	`}`
`372`	`372`	`if (xmlReader.attributes().hasAttribute("reporterrors")) {`
`373`		`- markup.reportErrors = (xmlReader.attributes().value("reporterrors") == QString("true")) ? true : false;`
	`373`	`+ markup.reportErrors = (xmlReader.attributes().value("reporterrors") == QString("true"));`
`374`	`374`	`} else {`
`375`	`375`	`mandatoryAttibuteMissing(xmlReader, "reporterrors");`
`376`	`376`	`}`
Original file line number	Diff line number	Diff line change
`@@ -147,10 +147,7 @@ Qt::CheckState SettingsDialog::boolToCheckState(bool yes)`
`147`	`147`
`148`	`148`	`bool SettingsDialog::checkStateToBool(Qt::CheckState state)`
`149`	`149`	`{`
`150`		`- if (state == Qt::Checked) {`
`151`		`- return true;`
`152`		`- }`
`153`		`- return false;`
	`150`	`+ return state == Qt::Checked;`
`154`	`151`	`}`
`155`	`152`
`156`	`153`
Original file line number	Diff line number	Diff line change
`@@ -554,10 +554,7 @@ void CheckOther::redundantAssignmentInSwitchError(const Token *tok1, const Token`
`554`	`554`	`//---------------------------------------------------------------------------`
`555`	`555`	`static inline bool isFunctionOrBreakPattern(const Token *tok)`
`556`	`556`	`{`
`557`		`- if (Token::Match(tok, "%name% (") \|\| Token::Match(tok, "break\|continue\|return\|exit\|goto\|throw"))`
`558`		`- return true;`
`559`		`-`
`560`		`- return false;`
	`557`	`+ return Token::Match(tok, "%name% (") \|\| Token::Match(tok, "break\|continue\|return\|exit\|goto\|throw");`
`561`	`558`	`}`
`562`	`559`
`563`	`560`	`void CheckOther::checkRedundantAssignmentInSwitch()`
`@@ -1097,7 +1094,7 @@ void CheckOther::variableScopeError(const Token *tok, const std::string &varname`
`1097`	`1094`	`void CheckOther::checkCommaSeparatedReturn()`
`1098`	`1095`	`{`
`1099`	`1096`	`// This is experimental for now. See #5076`
`1100`		`- if ((true) \|\| !mSettings->severity.isEnabled(Severity::style))`
	`1097`	`+ if ((true) \|\| !mSettings->severity.isEnabled(Severity::style)) // NOLINT(readability-simplify-boolean-expr)`
`1101`	`1098`	`return;`
`1102`	`1099`
`1103`	`1100`	`for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) {`
Original file line number	Diff line number	Diff line change
`@@ -702,9 +702,7 @@ static bool isSameIteratorContainerExpression(const Token* tok1,`
`702`	`702`	`ValueFlow::Value::LifetimeKind kind = ValueFlow::Value::LifetimeKind::Iterator)`
`703`	`703`	`{`
`704`	`704`	`if (isSameExpression(true, false, tok1, tok2, library, false, false)) {`
`705`		`- if (astIsContainerOwned(tok1) && isTemporary(true, tok1, &library))`
`706`		`- return false;`
`707`		`- return true;`
	`705`	`+ return !astIsContainerOwned(tok1) \|\| !isTemporary(true, tok1, &library);`
`708`	`706`	`}`
`709`	`707`	`if (kind == ValueFlow::Value::LifetimeKind::Address) {`
`710`	`708`	`return isSameExpression(true, false, getAddressContainer(tok1), getAddressContainer(tok2), library, false, false);`