Skip to content

Fix #11243 FN: knownConditionTrueFalse (range of typed enum) - #8930

Open
chrchr-github wants to merge 12 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_11243
Open

chrchr-github wants to merge 12 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_11243

Conversation

@chrchr-github

Copy link
Copy Markdown
Collaborator

No description provided.

@chrchr-github
chrchr-github marked this pull request as ready for review October 12, 2026 10:48

@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

The PR makes unsignedPositive and unsignedLessThanZero fire on comparisons of an enum with an unsigned underlying type against its first enumerator (c >= Color::Red, c <= Color::Red, Color::Red <= c). Base reports none of these. The edits to gui/mainwindow.cpp and lib/checkclass.cpp look like workarounds for exactly this in cppcheck's own code, but the PR has no test for the new behaviour and does not discuss it. I think these are false positives for a style check: the enumerator is zero only by accident of declaration order, and the message talks about "less than zero" / "negative" for an enum.

Smaller, also reproduced: for enum class K : char, the printf message now says the argument type is signed char, while a plain char argument is still reported as char.

The intended case works: e == 256 on a std::uint8_t enum is reported, including for forward-declared, namespaced, nested and typedef'd underlying types. The full testrunner passes.

Testing

Built cppcheck and testrunner in pr/ and cppcheck in base/ with make -j8. Ran the whole ./testrunner in pr/: 5346 tests, 0 failed. This includes the changed TestCondition, TestIO and TestSymbolDatabase. The GUI change was not built.

New unsignedPositive / unsignedLessThanZero on enums

t2.cpp:

#include <cstdint>
enum class Color : std::uint8_t { Red, Green, Blue, First = Red, Last = Blue };
bool isValid(Color c) { return c >= Color::First && c <= Color::Last; }
bool isFirst(Color c) { return c <= Color::Red; }
enum class Plain { Red, Green };
bool isValid2(Plain c) { return c >= Plain::Red; }
enum class U : unsigned { Red, Green };
bool isValid3(U c) { return c >= U::Red; }
bool u1(unsigned u) { return u <= 0; }
bool u2(unsigned u) { const unsigned zero = 0; return u >= zero; }
template <class T> bool t(T v) { return v >= T::Red; }
bool t2(Color c) { return t(c); }
bool loop() { for (Color c = Color::Blue; c >= Color::Red; c = static_cast<Color>(static_cast<int>(c) - 1)) {} return true; }
cppcheck --enable=style --inconclusive --library=std -q --template='{file}:{line}:{column}: {id}: {message}' t2.cpp

base:

t2.cpp:9:32: unsignedLessThanZero: Checking if unsigned expression 'u' is less than zero.
t2.cpp:10:57: unsignedPositive: Unsigned expression 'u' can't be negative so it is unnecessary to test it.

pr:

t2.cpp:4:34: unsignedLessThanZero: Checking if unsigned expression 'c' is less than zero.
t2.cpp:8:31: unsignedPositive: Unsigned expression 'c' can't be negative so it is unnecessary to test it.
t2.cpp:9:32: unsignedLessThanZero: Checking if unsigned expression 'u' is less than zero.
t2.cpp:10:57: unsignedPositive: Unsigned expression 'u' can't be negative so it is unnecessary to test it.
t2.cpp:13:45: unsignedPositive: Unsigned expression 'c' can't be negative so it is unnecessary to test it.

Lines 4, 8 and 13 are new. Line 8 shows it is not limited to library types: enum class U : unsigned is affected too, as is : unsigned char. Line 3, which compares against the alias Color::First, is not reported, so the result depends on which enumerator name is used.

In a second file the same new warnings appeared for:

  • member access and dereferences (s.r >= S::R::A, *p <= F::A, **pp >= F::A)
  • references
  • an anonymous enum : std::uint8_t {...} anon;
  • the reversed form Big::A <= b
  • enum class B : bool, where b >= B::A now gives knownConditionTrueFalse

signed char in the printf message

#include <cstdio>
enum class K : char { A, B };
void f7(K e, char c) { printf("%lu", e); printf("%lu", c); }

base:

t3.cpp:4:24: invalidPrintfArgType_uint: %lu in format string (no. 1) requires 'unsigned long' but the argument type is 'char'.
t3.cpp:4:42: invalidPrintfArgType_uint: %lu in format string (no. 1) requires 'unsigned long' but the argument type is 'char'.

pr:

t3.cpp:4:24: invalidPrintfArgType_uint: %lu in format string (no. 1) requires 'unsigned long' but the argument type is 'signed char'.
t3.cpp:4:42: invalidPrintfArgType_uint: %lu in format string (no. 1) requires 'unsigned long' but the argument type is 'char'.

Cases that behaved as intended

  • e == 256 is reported for enum E : std::uint8_t, for enum G : uint8_t, and for a forward-declared enum class Fwd : std::uint8_t;.
  • Underlying types std::int64_t and std::size_t now give signed long / unsigned long in printf messages.
  • An unknown underlying type (enum class N : unknown_t) and : wchar_t gave no crash and no new warnings.
  • In a .c file with enum E : uint8_t, neither base nor the PR reports anything.

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

Comment thread lib/symboldatabase.cpp
if (type->type()->classScope->type == ScopeType::eEnum) {
valuetype->sign = ValueType::Sign::SIGNED;
valuetype->type = getEnumType(type->type()->classScope, settings.platform);
if (valuetype->type == ValueType::Type::UNKNOWN_TYPE) {

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

Keeping the unsigned underlying type here makes checkSignOfUnsignedVariable() fire on enum comparisons against the first enumerator. Base reports none of these:

enum class Color : std::uint8_t { Red, Green, Blue };
bool isFirst(Color c) { return c <= Color::Red; }   // unsignedLessThanZero: Checking if unsigned expression 'c' is less than zero.
enum class U : unsigned { Red, Green };
bool isValid3(U c) { return c >= U::Red; }           // unsignedPositive: Unsigned expression 'c' can't be negative ...
for (Color c = Color::Blue; c >= Color::Red; ...)    // unsignedPositive

It also affects enum class X : unsigned char, which base did not warn about either. Comparing against an alias of the same value (First = Red, c >= Color::First) is not reported, so the result depends on the enumerator name.

I think these are false positives. The enumerator is zero only because it is declared first, the comparison is written in terms of enumerators and not 0, and range checks like c >= E::First are common. The message about "less than zero" / "negative" does not fit an enum.

Please consider skipping these checks when the zero operand is an enumerator, or when the other operand has an enum typeScope. Either way, add TestOther tests that pin down the intended behaviour.

Comment thread lib/checkclass.cpp

// Check if destructor is empty and non-empty ..
if (mSettings.standards.cpp <= Standards::CPP03) {
if (mSettings.standards.cpp == Standards::CPP03) {

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

This edit and the one in gui/mainwindow.cpp look like workarounds for the new unsignedLessThanZero that the PR produces on x <= Enum::FirstValue. User code with the same pattern will get the same warning (reproduced, see the comment in symboldatabase.cpp).

The original <= Standards::CPP03 was reasonable code. I would treat the need to rewrite it as a sign that the warning should be suppressed for enumerators, not as something to fix at each use.

Comment thread lib/symboldatabase.cpp Outdated
valuetype->sign = ValueType::Sign::SIGNED;
else if (enum_type->isUnsigned())
valuetype->sign = ValueType::Sign::UNSIGNED;
else if (!enum_type->isStandardType() && parsedecl(enum_type, &evt, defaultSignedness, settings))

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

For an enum whose underlying type is plain char, the reported type name changes from char to signed char:

enum class K : char { A, B };
void f(K e, char c) { printf("%lu", e); printf("%lu", c); }

Base says the argument type is 'char' for both calls. With the PR the first says 'signed char' and the second still says 'char'.

: char is a standard type, so it takes the defaultSignedness branch below, and the enum-typed variable now keeps that sign. The message is then inconsistent with how a plain char argument is reported.

@dmcppcheck

Copy link
Copy Markdown

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

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8930/
packages: 100
Differing warnings: 47 (+47 -0)
AI review: 1 improvement, 46 regression (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8930/2026-10-12_10-15-08-cabe20ef95f8-review.txt)
Timing: -0.1% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8930/2026-10-12_10-15-08-cabe20ef95f8_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

Copy link
Copy Markdown

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

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8930/
packages: 100
Differing warnings: 1 (+0 -1)
AI review: 1 improvement (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8930/2026-10-13_10-29-59-30df97b11fc5-review.txt)
Timing: -0.0% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8930/2026-10-13_10-29-59-30df97b11fc5_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

New regression: in parsedecl(), *valuetype = evt; replaces the whole ValueType, not just the type and sign. SymbolDatabase::setValueType(Token*, const Variable&) sets pointer (the array dimensions) and bits before it calls parsedecl(), and this assignment throws them away. As a result, an array of an enum whose underlying type is a library type is typed as a plain unsigned char instead of a pointer/array. For example, C a[3] with enum class C : std::uint8_t is affected. Base showed '*' for it.

I found one visible effect: constVariable is no longer reported for such arrays, while it is still reported for an otherwise identical enum based on unsigned char. Bitfield widths on such enum members are lost in the same way.

Other things I checked:

  • The intended e == 256 case works.
  • The new unsignedLessThanZero / unsignedPositive on c <= E::First, the checkclass.cpp / mainwindow.cpp workarounds and the signed char printf message still reproduce. The earlier review already raised these, so I don't repeat them here.
  • Function return values and casts to such an enum (get() == 256, static_cast<C>(i) == 256) are still typed signed int, the same as in base. That is a remaining false negative, not a regression.

Testing

Built cppcheck and testrunner in pr/ and cppcheck in base/ (make -j8). ./testrunner TestCondition TestIO TestSymbolDatabase TestOther TestType TestValueFlow: 893 tests, 0 failed.

Regression: arrays of an enum based on a library type

cv.cpp:

#include <cstdint>
enum class C : std::uint8_t { A, B };
enum class D : unsigned char { A, B };
bool f() {
    C a[3] = {};
    D b[3] = {};
    return a[0] == C::A && b[0] == D::A;
}

cppcheck --enable=style --library=std -q --template='{line}:{column}: {id}: {message}' cv.cpp

base:

5:7: constVariable: Variable 'a' can be declared as const array
6:7: constVariable: Variable 'b' can be declared as const array

pr:

6:7: constVariable: Variable 'b' can be declared as const array

--debug --verbose on C arr[3]; ... printf("%p", arr); auto p = arr; shows that the type is wrong. Base gives arr '*', p '*'. The PR gives arr 'unsigned char', p 'unsigned char', sizeof(r) with r 'unsigned char' for C (&r)[3], and s.c 'unsigned char' for a C c : 2 bitfield, where the bit width is dropped.

Other checks

  • e == 256 is reported for:
    • enum class C : std::uint8_t with a variable, a const C& or *p
    • enum G : n::T (a namespaced alias)
    • an underlying type that is a template parameter
  • There were no crashes on templates or typedefs.
  • A switch over all enumerators gives no new missingReturn, so the enum typeScope is restored after the overwrite.
  • get() == 256 and static_cast<C>(i) == 256 are still not reported, because the AST type is signed int. Base behaves the same.

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

Comment thread lib/symboldatabase.cpp
else if (!enum_type->isStandardType()) {
ValueType evt;
if (parsedecl(enum_type, &evt, defaultSignedness, settings))
*valuetype = evt;

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

*valuetype = evt; replaces everything the caller had already set. SymbolDatabase::setValueType(Token*, const Variable&) sets valuetype.pointer = var.dimensions().size() and valuetype.bits before it calls parsedecl(), and both are lost here. It also clears typeScope, container, smartPointerType and the debug path; typeScope is only restored later by the loop.

Effect: enum class C : std::uint8_t {A,B}; C a[3] = {}; now gives a the type unsigned char instead of a pointer/array. Base showed '*'. With that, constVariable ('a' can be declared as const array) is no longer reported, while the same code with enum class D : unsigned char still gets it. A C c : 2; bitfield loses its bit width in the same way.

Copying only type, sign and originalTypeName from evt would avoid this.

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.

3 participants