Skip to content

Fix #11592 FN compareValueOutOfTypeRangeError with char - #8940

Open
chrchr-github wants to merge 4 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_11592
Open

chrchr-github wants to merge 4 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_11592

Conversation

@chrchr-github

Copy link
Copy Markdown
Collaborator

No description provided.

@dmcppcheck

Copy link
Copy Markdown

Test results for commit ca3eda6 (tools/test-my-pr.py, main compared to this PR):

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8940/
packages: 100
Differing warnings: 1 (+1 -0)
AI review: 1 improvement (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8940/2026-10-14_11-03-25-ca3eda6b5fce-review.txt)
Timing: -0.1% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8940/2026-10-14_11-03-25-ca3eda6b5fce_timing.html)

Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong.

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

One new problem: a false positive on plain char compared with an unsigned value. When char is signed (unix64, native on x86), c > 127u and c > sizeof(...) are now reported as "always false". The usual arithmetic conversions turn a negative c into a huge unsigned value, so these conditions are true whenever c < 0. I confirmed this by compiling and running the code. The same false positive already exists for signed char and short, but the PR extends it to plain char, which is far more common, especially with sizeof operands. A fix would be to skip the relational operators (or allow for wraparound) when the value is unsigned with a rank of at least int and the type expression is signed. == and != against an unsigned value are still reported correctly.

Minor gap: the new condition only checks for a lowercase 's'. SymbolDatabase also accepts 'S', and with <default-sign>S</default-sign> in a platform file the new warning never appears.

Otherwise the new warnings for c == 128, c > 127, c == 255 and p[0] == 200 are correct on platforms where char is signed, and they correctly do not appear with <default-sign>u</default-sign>.

Testing

Builds and unit tests

  • Built cppcheck and testrunner in pr/, and cppcheck in base/.
  • ./testrunner TestCondition: 75 tests, 0 failures. ./testrunner TestOther TestType: 0 failures.

scratch/t1.cpp: plain, signed and unsigned char, with literal and pointer operands, on four platforms

Results on unix64 and native (x86):

  • The PR adds warnings for c == 128, c > 127, c == 255 and p[0] == 200 (const char *p). They are correct for a signed char.
  • c >= 127 is still not reported, which is also correct.

Platform files:

  • <default-sign>u</default-sign>: base and PR give the same output, as intended.
  • <default-sign>S</default-sign>: no new warnings, because only lowercase 's' is checked.

False positive with unsigned operands (scratch/t2.cpp)

void f(char c, signed char s, short sh) {
    if (c > 127u) {}
    if (c == 200u) {}
    if (s > 127u) {}
    if (sh > 32767u) {}
    if (c > sizeof(int) * 50) {}
}
int main(){ char c=-1; return c > 127u; }

Command:

cppcheck -q --enable=style --platform=unix64 t2.cpp

Output, base (only signed char and short, a false positive that already exists):

t2.cpp:4:13: style: Comparing expression of type 'signed char' against value 127. Condition is always false. [compareValueOutOfTypeRangeError]
t2.cpp:5:14: style: Comparing expression of type 'signed short' against value 32767. Condition is always false. [compareValueOutOfTypeRangeError]

Output, PR (adds these lines to base's two):

t2.cpp:2:13: style: Comparing expression of type 'char' against value 127. Condition is always false. [compareValueOutOfTypeRangeError]
t2.cpp:3:14: style: Comparing expression of type 'char' against value 200. Condition is always false. [compareValueOutOfTypeRangeError]
t2.cpp:6:25: style: Comparing expression of type 'char' against value 200. Condition is always false. [compareValueOutOfTypeRangeError]
t2.cpp:8:35: style: Comparing expression of type 'char' against value 127. Condition is always false. [compareValueOutOfTypeRangeError]
  • The warnings on lines 2, 6 and 8 are wrong. g++ t2.cpp && ./t2 exits with code 1, so c > 127u is true for c = -1.
  • The warning on line 3 (c == 200u) is correct.

Automated review of ca3eda6 by Claude. The pull request was built and tested locally.

Comment thread lib/checkcondition.cpp
const auto unsignedTypeMaxValue = (1LL << bits) - 1LL;
long long typeMaxValue;
if (typeTok->valueType()->sign != ValueType::Sign::SIGNED)
if (typeVt->sign != ValueType::Sign::SIGNED && !(typeVt->sign == ValueType::Sign::UNKNOWN_SIGN && typeVt->type == ValueType::Type::CHAR && mSettings.platform.defaultSign == 's'))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

Plain char now gets the signed range, but the operand conversions are not considered. When valueTok is unsigned with a rank of at least int (127u, sizeof(...)), a negative c becomes a huge unsigned value, so c > 127u is true for c == -1. The PR still reports it as always false:

int main(){ char c=-1; return c > 127u; }   // exits with 1 at runtime
void f(char c){ if (c > sizeof(int) * 50) {} }

--platform=unix64 gives Comparing expression of type 'char' against value 127. Condition is always false. and ... against value 200 .... Base already has this false positive for signed char and short, but this change extends it to plain char, where c > sizeof(...) is common. == and != against an unsigned value are still correct. The relational operators should be skipped, or should allow for wraparound, when the value operand is unsigned with a rank of at least int.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread lib/checkcondition.cpp
const auto unsignedTypeMaxValue = (1LL << bits) - 1LL;
long long typeMaxValue;
if (typeTok->valueType()->sign != ValueType::Sign::SIGNED)
if (typeVt->sign != ValueType::Sign::SIGNED && !(typeVt->sign == ValueType::Sign::UNKNOWN_SIGN && typeVt->type == ValueType::Type::CHAR && mSettings.platform.defaultSign == 's'))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

Only lowercase 's' is checked here. SymbolDatabase (lib/symboldatabase.cpp:67) also accepts 'S', and a platform file with <default-sign>S</default-sign> does not get the new warning (I confirmed this with c == 128).

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

New false positive: a plain char compared with a value cast to char. ValueFlow gives (char)200, static_cast<char>(0x80) and macros such as #define MARK ((char)0xE2) the unsigned values 200, 128 and 226. The cause is that ValueFlow::truncateIntValue (lib/vf_common.cpp:107) sign-extends only when dst_sign == SIGNED, and plain char has UNKNOWN_SIGN. Before this PR that did not matter, because the check took plain char to have a range of up to 255. Now that it uses a signed range when defaultSign == 's', c == (char)200 is reported as "always false". The condition is actually true for c = (char)200, and a compiled run confirms it. Comparing a char with a cast byte constant is a common pattern in UTF-8 and byte-parsing code. A fix is either to sign-extend UNKNOWN_SIGN char in the cast truncation when defaultSign is signed, or to skip the new signed range when the value operand is itself of type char.

The 'S' case and the unsigned-operand (c > 127u) false positive were already raised in comments.md, so I left them out.

Testing

  • Built cppcheck and testrunner in pr/, and cppcheck in base/. Both builds succeeded.
  • ./testrunner TestCondition: 75 tests, 0 failures.

False positive with a cast to char (scratch/b.cpp):

#define MARK ((char)0xE2)
int f(char c) {
    if (c == (char)200) return 1;
    if (c == static_cast<char>(0x80)) return 2;
    if (c == MARK) return 3;
    return 0;
}
int main() { return f((char)200) + 10*f((char)0x80); }

Command: cppcheck -q --enable=style --platform=unix64 b.cpp

Output with the PR:

b.cpp:3:14: style: Comparing expression of type 'char' against value 200. Condition is always false. [compareValueOutOfTypeRangeError]
b.cpp:4:31: style: Comparing expression of type 'char' against value 128. Condition is always false. [compareValueOutOfTypeRangeError]
b.cpp:5:14: style: Comparing expression of type 'char' against value 226. Condition is always false. [compareValueOutOfTypeRangeError]
  • Base gives no output for this file. The PR gives the same result with --platform=native.
  • g++ b.cpp -o b && ./b exits with code 21, so the conditions on lines 3 and 4 are true at runtime.
  • --debug-normal shows ( always 200 for (char)200. ValueFlow does not sign-extend the cast value for plain char.

Other checks

  • c == '\xff' and *p == '\xe2' give no warning, which is correct.
  • c == 0xff and c == L'\xff' are reported, which is correct.

Automated review of 3a9288b by Claude. The pull request was built and tested locally.

Comment thread lib/checkcondition.cpp
const auto unsignedTypeMaxValue = (1LL << bits) - 1LL;
long long typeMaxValue;
if (typeTok->valueType()->sign != ValueType::Sign::SIGNED)
if (typeVt->sign != ValueType::Sign::SIGNED && !(typeVt->sign == ValueType::Sign::UNKNOWN_SIGN && typeVt->type == ValueType::Type::CHAR && mSettings.platform.defaultSign == 's'))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

Plain char now gets the signed range, but ValueFlow does not sign-extend a value cast to plain char. ValueFlow::truncateIntValue (lib/vf_common.cpp:107) sign-extends only when the sign is SIGNED, so (char)200 becomes the known value 200 instead of -56. Comparisons like these are now reported as always false, but they can be true:

#define MARK ((char)0xE2)
int f(char c) {
    if (c == (char)200) return 1;              // "against value 200. Condition is always false"
    if (c == static_cast<char>(0x80)) return 2; // "against value 128 ..."
    if (c == MARK) return 3;                   // "against value 226 ..."
    return 0;
}

f((char)200) returns 1 when compiled with g++ on x86_64. Base does not warn here. One fix is to sign-extend UNKNOWN_SIGN char in the cast truncation when defaultSign is signed. Another is to skip this new case when valueTok is itself of type char.

@dmcppcheck

Copy link
Copy Markdown

Test results for commit 3a9288b (tools/test-my-pr.py, main compared to this PR):

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8940/
packages: 100
Differing warnings: 2 (+2 -0)
AI review: 2 improvement (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8940/2026-10-15_14-27-51-3a9288bd0dc8-review.txt)
Timing: -0.0% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8940/2026-10-15_14-27-51-3a9288bd0dc8_timing.html)

Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants