Skip to content

Commit a8b7e2b

Browse files
authored
Fix #13773 (False positive: should not warn when different ternary 2nd and 3rd expressions happens to have same value) (danmar#7922)
1 parent 74b2c43 commit a8b7e2b

File tree

4 files changed

+182
-24
lines changed

4 files changed

+182
-24
lines changed

lib/checkother.cpp

Lines changed: 26 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2163,7 +2163,7 @@ static bool isConstant(const Token* tok) {
21632163
return tok && (tok->isEnumerator() || Token::Match(tok, "%bool%|%num%|%str%|%char%|nullptr|NULL"));
21642164
}
21652165

2166-
static bool isConstStatement(const Token *tok, const Library& library, bool isNestedBracket = false)
2166+
static bool isConstStatement(const Token *tok, const Library& library, bool platformIndependent, bool isNestedBracket = false)
21672167
{
21682168
if (!tok)
21692169
return false;
@@ -2185,42 +2185,48 @@ static bool isConstStatement(const Token *tok, const Library& library, bool isNe
21852185
tok2 = tok2->astParent();
21862186
}
21872187
if (Token::Match(tok, "&&|%oror%"))
2188-
return isConstStatement(tok->astOperand1(), library) && isConstStatement(tok->astOperand2(), library);
2188+
return isConstStatement(tok->astOperand1(), library, platformIndependent) && isConstStatement(tok->astOperand2(), library, platformIndependent);
21892189
if (Token::Match(tok, "!|~|%cop%") && (tok->astOperand1() || tok->astOperand2()))
21902190
return true;
2191-
if (Token::Match(tok->previous(), "sizeof|alignof|noexcept|typeid (") && tok->previous()->isKeyword())
2191+
if (Token::simpleMatch(tok->previous(), "sizeof ("))
2192+
return !platformIndependent;
2193+
if (Token::Match(tok->previous(), "alignof|noexcept|typeid (") && tok->previous()->isKeyword())
21922194
return true;
21932195
if (isCPPCast(tok)) {
21942196
if (Token::simpleMatch(tok->astOperand1(), "dynamic_cast") && Token::simpleMatch(tok->astOperand1()->linkAt(1)->previous(), "& >"))
21952197
return false;
2196-
return isWithoutSideEffects(tok) && isConstStatement(tok->astOperand2(), library);
2198+
return isWithoutSideEffects(tok) && isConstStatement(tok->astOperand2(), library, platformIndependent);
21972199
}
21982200
if (tok->isCast() && tok->next() && tok->next()->isStandardType())
2199-
return isWithoutSideEffects(tok->astOperand1()) && isConstStatement(tok->astOperand1(), library);
2201+
return isWithoutSideEffects(tok->astOperand1()) && isConstStatement(tok->astOperand1(), library, platformIndependent);
22002202
if (Token::simpleMatch(tok, "."))
2201-
return isConstStatement(tok->astOperand2(), library);
2203+
return isConstStatement(tok->astOperand2(), library, platformIndependent);
22022204
if (Token::simpleMatch(tok, ",")) {
22032205
if (tok->astParent()) // warn about const statement on rhs at the top level
2204-
return isConstStatement(tok->astOperand1(), library) && isConstStatement(tok->astOperand2(), library);
2206+
return isConstStatement(tok->astOperand1(), library, platformIndependent) &&
2207+
isConstStatement(tok->astOperand2(), library, platformIndependent);
22052208

22062209
const Token* lml = previousBeforeAstLeftmostLeaf(tok); // don't warn about matrix/vector assignment (e.g. Eigen)
22072210
if (lml)
22082211
lml = lml->next();
22092212
const Token* stream = lml;
22102213
while (stream && Token::Match(stream->astParent(), ".|[|(|*"))
22112214
stream = stream->astParent();
2212-
return (!stream || !isLikelyStream(stream)) && isConstStatement(tok->astOperand2(), library);
2215+
return (!stream || !isLikelyStream(stream)) && isConstStatement(tok->astOperand2(), library, platformIndependent);
22132216
}
22142217
if (Token::simpleMatch(tok, "?") && Token::simpleMatch(tok->astOperand2(), ":")) // ternary operator
2215-
return isConstStatement(tok->astOperand1(), library) && isConstStatement(tok->astOperand2()->astOperand1(), library) && isConstStatement(tok->astOperand2()->astOperand2(), library);
2218+
return isConstStatement(tok->astOperand1(), library, platformIndependent) &&
2219+
isConstStatement(tok->astOperand2()->astOperand1(), library, platformIndependent) &&
2220+
isConstStatement(tok->astOperand2()->astOperand2(), library, platformIndependent);
22162221
if (isBracketAccess(tok) && isWithoutSideEffects(tok->astOperand1(), /*checkArrayAccess*/ true, /*checkReference*/ false)) {
22172222
const bool isChained = succeeds(tok->astParent(), tok);
22182223
if (Token::simpleMatch(tok->astParent(), "[")) {
22192224
if (isChained)
2220-
return isConstStatement(tok->astOperand2(), library) && isConstStatement(tok->astParent(), library);
2221-
return isNestedBracket && isConstStatement(tok->astOperand2(), library);
2225+
return isConstStatement(tok->astOperand2(), library, platformIndependent) &&
2226+
isConstStatement(tok->astParent(), library, platformIndependent);
2227+
return isNestedBracket && isConstStatement(tok->astOperand2(), library, platformIndependent);
22222228
}
2223-
return isConstStatement(tok->astOperand2(), library, /*isNestedBracket*/ !isChained);
2229+
return isConstStatement(tok->astOperand2(), library, platformIndependent, /*isNestedBracket*/ !isChained);
22242230
}
22252231
if (!tok->astParent() && findLambdaEndToken(tok))
22262232
return true;
@@ -2331,7 +2337,7 @@ void CheckOther::checkIncompleteStatement()
23312337
// Skip statement expressions
23322338
if (Token::simpleMatch(rtok, "; } )"))
23332339
continue;
2334-
if (!isConstStatement(tok, mSettings->library))
2340+
if (!isConstStatement(tok, mSettings->library, false))
23352341
continue;
23362342
if (isVoidStmt(tok))
23372343
continue;
@@ -2526,7 +2532,7 @@ void CheckOther::checkMisusedScopedObject()
25262532
if (Token::simpleMatch(parTok, "<") && parTok->link())
25272533
parTok = parTok->link()->next();
25282534
if (const Token* arg = parTok->astOperand2()) {
2529-
if (!isConstStatement(arg, mSettings->library))
2535+
if (!isConstStatement(arg, mSettings->library, false))
25302536
continue;
25312537
if (parTok->str() == "(") {
25322538
if (arg->varId() && !(arg->variable() && arg->variable()->nameToken() != arg))
@@ -2985,12 +2991,13 @@ void CheckOther::checkDuplicateExpression()
29852991
}
29862992
}
29872993
} else if (tok->astOperand1() && tok->astOperand2() && tok->str() == ":" && tok->astParent() && tok->astParent()->str() == "?") {
2988-
if (!tok->astOperand1()->values().empty() && !tok->astOperand2()->values().empty() && isEqualKnownValue(tok->astOperand1(), tok->astOperand2()) &&
2989-
!isVariableChanged(tok->astParent(), /*indirect*/ 0, *mSettings) &&
2990-
isConstStatement(tok->astOperand1(), mSettings->library) && isConstStatement(tok->astOperand2(), mSettings->library))
2991-
duplicateValueTernaryError(tok);
2992-
else if (isSameExpression(true, tok->astOperand1(), tok->astOperand2(), *mSettings, false, true, &errorPath))
2994+
if (isSameExpression(true, tok->astOperand1(), tok->astOperand2(), *mSettings, false, true, &errorPath))
29932995
duplicateExpressionTernaryError(tok, std::move(errorPath));
2996+
2997+
else if (!tok->astOperand1()->values().empty() && !tok->astOperand2()->values().empty() && isEqualKnownValue(tok->astOperand1(), tok->astOperand2()) &&
2998+
!isVariableChanged(tok->astParent(), /*indirect*/ 0, *mSettings) &&
2999+
isConstStatement(tok->astOperand1(), mSettings->library, true) && isConstStatement(tok->astOperand2(), mSettings->library, true))
3000+
duplicateValueTernaryError(tok);
29943001
}
29953002
}
29963003
}
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
# duplicateExpressionTernary
2+
3+
**Message**: Same expression in both branches of ternary operator<br/>
4+
**Category**: Logic<br/>
5+
**Severity**: Style<br/>
6+
**Language**: C/C++
7+
8+
## Description
9+
10+
This checker detects when syntactically identical expressions appear in both the true and false branches of a ternary operator (`condition ? true_expr : false_expr`).
11+
12+
The warning is triggered when:
13+
- The second and third operands of a ternary operator are syntactically identical expressions
14+
- This includes cases where variables are referenced through aliases that resolve to the same expression
15+
16+
## Motivation
17+
18+
The same expression indicates that there might be some logic error or copy-paste mistake.
19+
20+
## Examples
21+
22+
### Problematic code
23+
24+
```cpp
25+
// Same expression in both branches
26+
int result = condition ? x : x; // Warning: duplicateExpressionTernary
27+
28+
// Same variable referenced through alias
29+
const int c = a;
30+
int result = condition ? a : c; // Warning: duplicateExpressionTernary
31+
```
32+
33+
### Fixed code
34+
35+
```cpp
36+
// Different expressions in branches
37+
int result = condition ? x : y; // OK
38+
```
39+
40+
## How to fix
41+
42+
1. **Check for copy-paste errors**: Verify that both branches are supposed to have the same expression
43+
2. **Use different expressions**: If the branches should differ, update one of them
44+
3. **Simplify the code**: If both branches are intentionally the same, remove the ternary operator entirely
45+
46+
Before:
47+
```cpp
48+
int getValue(bool flag) {
49+
return flag ? computeValue() : computeValue(); // Same computation in both branches
50+
}
51+
```
52+
53+
After:
54+
```cpp
55+
int getValue(bool flag) {
56+
return computeValue(); // Simplified - condition doesn't matter
57+
}
58+
```
59+
60+
Or if the branches should differ:
61+
```cpp
62+
int getValue(bool flag) {
63+
return flag ? computeValue() : computeAlternativeValue(); // Different computations
64+
}
65+
```
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
# duplicateValueTernary
2+
3+
**Message**: Same value in both branches of ternary operator<br/>
4+
**Category**: Logic<br/>
5+
**Severity**: Style<br/>
6+
**Language**: C/C++
7+
8+
## Description
9+
10+
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.
11+
12+
The warning is triggered when:
13+
- The second and third operands of a ternary operator evaluate to the same constant value
14+
- The expressions are constant statements that don't change during evaluation
15+
- The expressions have different syntax but equivalent values
16+
17+
However, no warning is generated when:
18+
- The expressions have different values on different platforms
19+
- Variables are modified between evaluations
20+
- The expressions involve non-constant computations
21+
22+
## Motivation
23+
24+
The same value indicates that there might be some logic error or copy-paste mistake.
25+
26+
## Examples
27+
28+
### Problematic code
29+
30+
```cpp
31+
// Different expressions, same value
32+
int result = condition ? (int)1 : 1; // Warning: duplicateValueTernary
33+
34+
// Different cast syntax, same value
35+
int result = condition ? 1 : (int)1; // Warning: duplicateValueTernary
36+
```
37+
38+
### Fixed code
39+
40+
```cpp
41+
// Different values in branches
42+
int result = condition ? 1 : 2; // OK
43+
44+
// Simplified - condition doesn't matter
45+
int result = 1; // OK - removed unnecessary ternary
46+
47+
// Platform-dependent values are allowed
48+
int size = is_64bit ? sizeof(long) : sizeof(int); // OK - may differ on platforms
49+
```
50+
51+
## How to fix
52+
53+
1. **Check for logic errors**: Verify if both branches should actually have the same value
54+
2. **Simplify the code**: If both branches always have the same value, remove the ternary operator
55+
3. **Use different values**: If the branches should differ, update one of them
56+
4. **Remove unnecessary casts**: If the difference is only in casting, consider if the cast is needed
57+
58+
Before:
59+
```cpp
60+
int getValue(bool flag) {
61+
return flag ? (int)42 : 42; // Same value, different expressions
62+
}
63+
```
64+
65+
After (simplified):
66+
```cpp
67+
int getValue(bool flag) {
68+
return 42; // Simplified - condition doesn't affect result
69+
}
70+
```
71+
72+
Or (if branches should differ):
73+
```cpp
74+
int getValue(bool flag) {
75+
return flag ? 42 : 0; // Different values for different conditions
76+
}
77+
```

test/testother.cpp

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,7 @@ class TestOther : public TestFixture {
197197
TEST_CASE(duplicateExpression18);
198198
TEST_CASE(duplicateExpressionLoop);
199199
TEST_CASE(duplicateValueTernary);
200+
TEST_CASE(duplicateValueTernarySizeof); // #13773
200201
TEST_CASE(duplicateExpressionTernary); // #6391
201202
TEST_CASE(duplicateExpressionTemplate); // #6930
202203
TEST_CASE(duplicateExpressionCompareWithZero);
@@ -8106,31 +8107,31 @@ class TestOther : public TestFixture {
81068107
check("void f() {\n"
81078108
" if( a ? (b ? false:false): false ) ;\n"
81088109
"}");
8109-
ASSERT_EQUALS("[test.cpp:2:23]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str());
8110+
ASSERT_EQUALS("[test.cpp:2:23]: (style) Same expression in both branches of ternary operator. [duplicateExpressionTernary]\n", errout_str());
81108111

81118112
check("int f1(int a) {return (a == 1) ? (int)1 : 1; }");
81128113
ASSERT_EQUALS("[test.cpp:1:41]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str());
81138114

81148115
check("int f2(int a) {return (a == 1) ? (int)1 : (int)1; }");
8115-
ASSERT_EQUALS("[test.cpp:1:41]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str());
8116+
ASSERT_EQUALS("[test.cpp:1:41]: (style) Same expression in both branches of ternary operator. [duplicateExpressionTernary]\n", errout_str());
81168117

81178118
check("int f3(int a) {return (a == 1) ? 1 : (int)1; }");
81188119
ASSERT_EQUALS("[test.cpp:1:36]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str());
81198120

81208121
check("int f4(int a) {return (a == 1) ? 1 : 1; }");
8121-
ASSERT_EQUALS("[test.cpp:1:36]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str());
8122+
ASSERT_EQUALS("[test.cpp:1:36]: (style) Same expression in both branches of ternary operator. [duplicateExpressionTernary]\n", errout_str());
81228123

81238124
check("int f5(int a) {return (a == (int)1) ? (int)1 : 1; }");
81248125
ASSERT_EQUALS("[test.cpp:1:46]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str());
81258126

81268127
check("int f6(int a) {return (a == (int)1) ? (int)1 : (int)1; }");
8127-
ASSERT_EQUALS("[test.cpp:1:46]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str());
8128+
ASSERT_EQUALS("[test.cpp:1:46]: (style) Same expression in both branches of ternary operator. [duplicateExpressionTernary]\n", errout_str());
81288129

81298130
check("int f7(int a) {return (a == (int)1) ? 1 : (int)1; }");
81308131
ASSERT_EQUALS("[test.cpp:1:41]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str());
81318132

81328133
check("int f8(int a) {return (a == (int)1) ? 1 : 1; }");
8133-
ASSERT_EQUALS("[test.cpp:1:41]: (style) Same value in both branches of ternary operator. [duplicateValueTernary]\n", errout_str());
8134+
ASSERT_EQUALS("[test.cpp:1:41]: (style) Same expression in both branches of ternary operator. [duplicateExpressionTernary]\n", errout_str());
81348135

81358136
check("struct Foo {\n"
81368137
" std::vector<int> bar{1,2,3};\n"
@@ -8170,6 +8171,14 @@ class TestOther : public TestFixture {
81708171
ASSERT_EQUALS("", errout_str());
81718172
}
81728173

8174+
void duplicateValueTernarySizeof() { // #13773
8175+
check("int f() { return x ? sizeof(uint32_t) : sizeof(unsigned int); }");
8176+
ASSERT_EQUALS("", errout_str());
8177+
8178+
check("int f() { return x ? sizeof(uint32_t) : sizeof(uint32_t); }");
8179+
ASSERT_EQUALS("[test.cpp:1:39]: (style) Same expression in both branches of ternary operator. [duplicateExpressionTernary]\n", errout_str());
8180+
}
8181+
81738182
void duplicateExpressionTemplate() {
81748183
check("template <int I> void f() {\n" // #6930
81758184
" if (I >= 0 && I < 3) {}\n"

0 commit comments

Comments
 (0)