Skip to content

Fix #11592 FN compareValueOutOfTypeRangeError with char - #8940

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

chrchr-github wants to merge 3 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.

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).

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