diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 871b5905db8..d794bf92736 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2163,7 +2163,7 @@ static bool isConstant(const Token* tok) { return tok && (tok->isEnumerator() || Token::Match(tok, "%bool%|%num%|%str%|%char%|nullptr|NULL")); } -static bool isConstStatement(const Token *tok, const Library& library, bool isNestedBracket = false) +static bool isConstStatement(const Token *tok, const Library& library, bool platformIndependent, bool isNestedBracket = false) { if (!tok) return false; @@ -2185,23 +2185,26 @@ static bool isConstStatement(const Token *tok, const Library& library, bool isNe tok2 = tok2->astParent(); } if (Token::Match(tok, "&&|%oror%")) - return isConstStatement(tok->astOperand1(), library) && isConstStatement(tok->astOperand2(), library); + return isConstStatement(tok->astOperand1(), library, platformIndependent) && isConstStatement(tok->astOperand2(), library, platformIndependent); if (Token::Match(tok, "!|~|%cop%") && (tok->astOperand1() || tok->astOperand2())) return true; - if (Token::Match(tok->previous(), "sizeof|alignof|noexcept|typeid (") && tok->previous()->isKeyword()) + if (Token::simpleMatch(tok->previous(), "sizeof (")) + return !platformIndependent; + if (Token::Match(tok->previous(), "alignof|noexcept|typeid (") && tok->previous()->isKeyword()) return true; if (isCPPCast(tok)) { if (Token::simpleMatch(tok->astOperand1(), "dynamic_cast") && Token::simpleMatch(tok->astOperand1()->linkAt(1)->previous(), "& >")) return false; - return isWithoutSideEffects(tok) && isConstStatement(tok->astOperand2(), library); + return isWithoutSideEffects(tok) && isConstStatement(tok->astOperand2(), library, platformIndependent); } if (tok->isCast() && tok->next() && tok->next()->isStandardType()) - return isWithoutSideEffects(tok->astOperand1()) && isConstStatement(tok->astOperand1(), library); + return isWithoutSideEffects(tok->astOperand1()) && isConstStatement(tok->astOperand1(), library, platformIndependent); if (Token::simpleMatch(tok, ".")) - return isConstStatement(tok->astOperand2(), library); + return isConstStatement(tok->astOperand2(), library, platformIndependent); if (Token::simpleMatch(tok, ",")) { if (tok->astParent()) // warn about const statement on rhs at the top level - return isConstStatement(tok->astOperand1(), library) && isConstStatement(tok->astOperand2(), library); + return isConstStatement(tok->astOperand1(), library, platformIndependent) && + isConstStatement(tok->astOperand2(), library, platformIndependent); const Token* lml = previousBeforeAstLeftmostLeaf(tok); // don't warn about matrix/vector assignment (e.g. Eigen) if (lml) @@ -2209,18 +2212,21 @@ static bool isConstStatement(const Token *tok, const Library& library, bool isNe const Token* stream = lml; while (stream && Token::Match(stream->astParent(), ".|[|(|*")) stream = stream->astParent(); - return (!stream || !isLikelyStream(stream)) && isConstStatement(tok->astOperand2(), library); + return (!stream || !isLikelyStream(stream)) && isConstStatement(tok->astOperand2(), library, platformIndependent); } if (Token::simpleMatch(tok, "?") && Token::simpleMatch(tok->astOperand2(), ":")) // ternary operator - return isConstStatement(tok->astOperand1(), library) && isConstStatement(tok->astOperand2()->astOperand1(), library) && isConstStatement(tok->astOperand2()->astOperand2(), library); + return isConstStatement(tok->astOperand1(), library, platformIndependent) && + isConstStatement(tok->astOperand2()->astOperand1(), library, platformIndependent) && + isConstStatement(tok->astOperand2()->astOperand2(), library, platformIndependent); if (isBracketAccess(tok) && isWithoutSideEffects(tok->astOperand1(), /*checkArrayAccess*/ true, /*checkReference*/ false)) { const bool isChained = succeeds(tok->astParent(), tok); if (Token::simpleMatch(tok->astParent(), "[")) { if (isChained) - return isConstStatement(tok->astOperand2(), library) && isConstStatement(tok->astParent(), library); - return isNestedBracket && isConstStatement(tok->astOperand2(), library); + return isConstStatement(tok->astOperand2(), library, platformIndependent) && + isConstStatement(tok->astParent(), library, platformIndependent); + return isNestedBracket && isConstStatement(tok->astOperand2(), library, platformIndependent); } - return isConstStatement(tok->astOperand2(), library, /*isNestedBracket*/ !isChained); + return isConstStatement(tok->astOperand2(), library, platformIndependent, /*isNestedBracket*/ !isChained); } if (!tok->astParent() && findLambdaEndToken(tok)) return true; @@ -2331,7 +2337,7 @@ void CheckOther::checkIncompleteStatement() // Skip statement expressions if (Token::simpleMatch(rtok, "; } )")) continue; - if (!isConstStatement(tok, mSettings->library)) + if (!isConstStatement(tok, mSettings->library, false)) continue; if (isVoidStmt(tok)) continue; @@ -2526,7 +2532,7 @@ void CheckOther::checkMisusedScopedObject() if (Token::simpleMatch(parTok, "<") && parTok->link()) parTok = parTok->link()->next(); if (const Token* arg = parTok->astOperand2()) { - if (!isConstStatement(arg, mSettings->library)) + if (!isConstStatement(arg, mSettings->library, false)) continue; if (parTok->str() == "(") { if (arg->varId() && !(arg->variable() && arg->variable()->nameToken() != arg)) @@ -2985,12 +2991,13 @@ void CheckOther::checkDuplicateExpression() } } } else if (tok->astOperand1() && tok->astOperand2() && tok->str() == ":" && tok->astParent() && tok->astParent()->str() == "?") { - if (!tok->astOperand1()->values().empty() && !tok->astOperand2()->values().empty() && isEqualKnownValue(tok->astOperand1(), tok->astOperand2()) && - !isVariableChanged(tok->astParent(), /*indirect*/ 0, *mSettings) && - isConstStatement(tok->astOperand1(), mSettings->library) && isConstStatement(tok->astOperand2(), mSettings->library)) - duplicateValueTernaryError(tok); - else if (isSameExpression(true, tok->astOperand1(), tok->astOperand2(), *mSettings, false, true, &errorPath)) + if (isSameExpression(true, tok->astOperand1(), tok->astOperand2(), *mSettings, false, true, &errorPath)) duplicateExpressionTernaryError(tok, std::move(errorPath)); + + else if (!tok->astOperand1()->values().empty() && !tok->astOperand2()->values().empty() && isEqualKnownValue(tok->astOperand1(), tok->astOperand2()) && + !isVariableChanged(tok->astParent(), /*indirect*/ 0, *mSettings) && + isConstStatement(tok->astOperand1(), mSettings->library, true) && isConstStatement(tok->astOperand2(), mSettings->library, true)) + duplicateValueTernaryError(tok); } } } diff --git a/man/checkers/duplicateExpressionTernary.md b/man/checkers/duplicateExpressionTernary.md new file mode 100644 index 00000000000..706ad85e2ac --- /dev/null +++ b/man/checkers/duplicateExpressionTernary.md @@ -0,0 +1,65 @@ +# duplicateExpressionTernary + +**Message**: Same expression in both branches of ternary operator
+**Category**: Logic
+**Severity**: Style
+**Language**: C/C++ + +## Description + +This checker detects when syntactically identical expressions appear in both the true and false branches of a ternary operator (`condition ? true_expr : false_expr`). + +The warning is triggered when: +- The second and third operands of a ternary operator are syntactically identical expressions +- This includes cases where variables are referenced through aliases that resolve to the same expression + +## Motivation + +The same expression indicates that there might be some logic error or copy-paste mistake. + +## Examples + +### Problematic code + +```cpp +// Same expression in both branches +int result = condition ? x : x; // Warning: duplicateExpressionTernary + +// Same variable referenced through alias +const int c = a; +int result = condition ? a : c; // Warning: duplicateExpressionTernary +``` + +### Fixed code + +```cpp +// Different expressions in branches +int result = condition ? x : y; // OK +``` + +## How to fix + +1. **Check for copy-paste errors**: Verify that both branches are supposed to have the same expression +2. **Use different expressions**: If the branches should differ, update one of them +3. **Simplify the code**: If both branches are intentionally the same, remove the ternary operator entirely + +Before: +```cpp +int getValue(bool flag) { + return flag ? computeValue() : computeValue(); // Same computation in both branches +} +``` + +After: +```cpp +int getValue(bool flag) { + return computeValue(); // Simplified - condition doesn't matter +} +``` + +Or if the branches should differ: +```cpp +int getValue(bool flag) { + return flag ? computeValue() : computeAlternativeValue(); // Different computations +} +``` diff --git a/man/checkers/duplicateValueTernary.md b/man/checkers/duplicateValueTernary.md new file mode 100644 index 00000000000..06e9f8e9be7 --- /dev/null +++ b/man/checkers/duplicateValueTernary.md @@ -0,0 +1,77 @@ +# duplicateValueTernary + +**Message**: Same value in both branches of ternary operator
+**Category**: Logic
+**Severity**: Style
+**Language**: C/C++ + +## Description + +This checker detects when both branches of a ternary operator (`condition ? true_expr : false_expr`) evaluate to the same value, even though the expressions themselves may be syntactically different. + +The warning is triggered when: +- The second and third operands of a ternary operator evaluate to the same constant value +- The expressions are constant statements that don't change during evaluation +- The expressions have different syntax but equivalent values + +However, no warning is generated when: +- The expressions have different values on different platforms +- Variables are modified between evaluations +- The expressions involve non-constant computations + +## Motivation + +The same value indicates that there might be some logic error or copy-paste mistake. + +## Examples + +### Problematic code + +```cpp +// Different expressions, same value +int result = condition ? (int)1 : 1; // Warning: duplicateValueTernary + +// Different cast syntax, same value +int result = condition ? 1 : (int)1; // Warning: duplicateValueTernary +``` + +### Fixed code + +```cpp +// Different values in branches +int result = condition ? 1 : 2; // OK + +// Simplified - condition doesn't matter +int result = 1; // OK - removed unnecessary ternary + +// Platform-dependent values are allowed +int size = is_64bit ? sizeof(long) : sizeof(int); // OK - may differ on platforms +``` + +## How to fix + +1. **Check for logic errors**: Verify if both branches should actually have the same value +2. **Simplify the code**: If both branches always have the same value, remove the ternary operator +3. **Use different values**: If the branches should differ, update one of them +4. **Remove unnecessary casts**: If the difference is only in casting, consider if the cast is needed + +Before: +```cpp +int getValue(bool flag) { + return flag ? (int)42 : 42; // Same value, different expressions +} +``` + +After (simplified): +```cpp +int getValue(bool flag) { + return 42; // Simplified - condition doesn't affect result +} +``` + +Or (if branches should differ): +```cpp +int getValue(bool flag) { + return flag ? 42 : 0; // Different values for different conditions +} +``` diff --git a/test/testother.cpp b/test/testother.cpp index 6a0957e7830..c980151b3fc 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -197,6 +197,7 @@ class TestOther : public TestFixture { TEST_CASE(duplicateExpression18); TEST_CASE(duplicateExpressionLoop); TEST_CASE(duplicateValueTernary); + TEST_CASE(duplicateValueTernarySizeof); // #13773 TEST_CASE(duplicateExpressionTernary); // #6391 TEST_CASE(duplicateExpressionTemplate); // #6930 TEST_CASE(duplicateExpressionCompareWithZero); @@ -8106,31 +8107,31 @@ class TestOther : public TestFixture { check("void f() {\n" " if( a ? (b ? false:false): false ) ;\n" "}"); - ASSERT_EQUALS("[test.cpp:2:23]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str()); + ASSERT_EQUALS("[test.cpp:2:23]: (style) Same expression in both branches of ternary operator. [duplicateExpressionTernary]\n", errout_str()); check("int f1(int a) {return (a == 1) ? (int)1 : 1; }"); ASSERT_EQUALS("[test.cpp:1:41]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str()); check("int f2(int a) {return (a == 1) ? (int)1 : (int)1; }"); - ASSERT_EQUALS("[test.cpp:1:41]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str()); + ASSERT_EQUALS("[test.cpp:1:41]: (style) Same expression in both branches of ternary operator. [duplicateExpressionTernary]\n", errout_str()); check("int f3(int a) {return (a == 1) ? 1 : (int)1; }"); ASSERT_EQUALS("[test.cpp:1:36]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str()); check("int f4(int a) {return (a == 1) ? 1 : 1; }"); - ASSERT_EQUALS("[test.cpp:1:36]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str()); + ASSERT_EQUALS("[test.cpp:1:36]: (style) Same expression in both branches of ternary operator. [duplicateExpressionTernary]\n", errout_str()); check("int f5(int a) {return (a == (int)1) ? (int)1 : 1; }"); ASSERT_EQUALS("[test.cpp:1:46]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str()); check("int f6(int a) {return (a == (int)1) ? (int)1 : (int)1; }"); - ASSERT_EQUALS("[test.cpp:1:46]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str()); + ASSERT_EQUALS("[test.cpp:1:46]: (style) Same expression in both branches of ternary operator. [duplicateExpressionTernary]\n", errout_str()); check("int f7(int a) {return (a == (int)1) ? 1 : (int)1; }"); ASSERT_EQUALS("[test.cpp:1:41]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str()); check("int f8(int a) {return (a == (int)1) ? 1 : 1; }"); - ASSERT_EQUALS("[test.cpp:1:41]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str()); + ASSERT_EQUALS("[test.cpp:1:41]: (style) Same expression in both branches of ternary operator. [duplicateExpressionTernary]\n", errout_str()); check("struct Foo {\n" " std::vector bar{1,2,3};\n" @@ -8170,6 +8171,14 @@ class TestOther : public TestFixture { ASSERT_EQUALS("", errout_str()); } + void duplicateValueTernarySizeof() { // #13773 + check("int f() { return x ? sizeof(uint32_t) : sizeof(unsigned int); }"); + ASSERT_EQUALS("", errout_str()); + + check("int f() { return x ? sizeof(uint32_t) : sizeof(uint32_t); }"); + ASSERT_EQUALS("[test.cpp:1:39]: (style) Same expression in both branches of ternary operator. [duplicateExpressionTernary]\n", errout_str()); + } + void duplicateExpressionTemplate() { check("template void f() {\n" // #6930 " if (I >= 0 && I < 3) {}\n"