Repository navigation
Fix #11243 FN: knownConditionTrueFalse (range of typed enum) - #8930
chrchr-github wants to merge 12 commits into
Conversation
dmcppcheck
left a comment
There was a problem hiding this comment.
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, whereb >= B::Anow givesknownConditionTrueFalse
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 == 256is reported forenum E : std::uint8_t, forenum G : uint8_t, and for a forward-declaredenum class Fwd : std::uint8_t;.- Underlying types
std::int64_tandstd::size_tnow givesigned long/unsigned longin printf messages. - An unknown underlying type (
enum class N : unknown_t) and: wchar_tgave no crash and no new warnings. - In a
.cfile withenum E : uint8_t, neither base nor the PR reports anything.
Automated review of cabe20e by Claude. The pull request was built and tested locally.
| 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) { |
There was a problem hiding this comment.
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; ...) // unsignedPositiveIt 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.
|
|
||
| // Check if destructor is empty and non-empty .. | ||
| if (mSettings.standards.cpp <= Standards::CPP03) { | ||
| if (mSettings.standards.cpp == Standards::CPP03) { |
There was a problem hiding this comment.
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.
| 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)) |
There was a problem hiding this comment.
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.
|
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/ 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. |
|
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/ 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
left a comment
There was a problem hiding this comment.
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 == 256case works. - The new
unsignedLessThanZero/unsignedPositiveonc <= E::First, thecheckclass.cpp/mainwindow.cppworkarounds and thesigned charprintf 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 typedsigned 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 == 256is reported for:enum class C : std::uint8_twith a variable, aconst C&or*penum G : n::T(a namespaced alias)- an underlying type that is a template parameter
- There were no crashes on templates or typedefs.
- A
switchover all enumerators gives no newmissingReturn, so the enumtypeScopeis restored after the overwrite. get() == 256andstatic_cast<C>(i) == 256are still not reported, because the AST type issigned int. Base behaves the same.
Automated review of 30df97b by Claude. The pull request was built and tested locally.
| else if (!enum_type->isStandardType()) { | ||
| ValueType evt; | ||
| if (parsedecl(enum_type, &evt, defaultSignedness, settings)) | ||
| *valuetype = evt; |
There was a problem hiding this comment.
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.
No description provided.