fixed #13363 - apply default signedness to char only#7155
fixed #13363 - apply default signedness to char only#7155firewave merged 1 commit intocppcheck-opensource:mainfrom
Conversation
firewave
force-pushed
the
default-sign-x
branch
2 times, most recently
from
2b43e52 to
9a6bca1
Compare
danmar
left a comment
danmar
left a comment
There was a problem hiding this comment.
yes the default sign should only be applied to char
firewave
force-pushed
the
default-sign-x
branch
from
9a6bca1 to
f130ade
Compare
| valuetype->sign = ValueType::Sign::UNSIGNED; | ||
| else | ||
| valuetype->sign = defaultSignedness; | ||
| valuetype->sign = defaultSignedness; // TODO: this is implementation-dependent might be separate from char |
There was a problem hiding this comment.
What do you mean? I feel there is something I don't understand.
enum {A,B,C} abc;
if (abc == A) {} // <- A is an 'int' right?
There was a problem hiding this comment.
It is an integer but not necessarily int.
From https://en.cppreference.com/w/cpp/language/enum:
the underlying type is an implementation-defined integral type that can represent all enumerator values; this type is not larger than int unless the value of an enumerator cannot fit in an int or unsigned int
Also https://stackoverflow.com/a/1122128/532627 for an actual reference to the standard.
It should definitely not depend on the flag we use for char which is configurable via compiler flags.
There was a problem hiding this comment.
It is an integer but not necessarily int.
ok I see.
It should definitely not depend on the flag we use for char which is configurable via compiler flags.
I agree.
There was a problem hiding this comment.
I filed https://trac.cppcheck.net/ticket/13602 about this.
danmar
left a comment
danmar
left a comment
There was a problem hiding this comment.
well I would have liked if you took care of the enums at once in some reasonable way directly. imho it would be reasonable to make it "signed int" by default and we could go for "unsigned" if some enum constant would require it..
but this is an improvement at least so feel free to merge..
That requires some tests for our code and with the existing compilers out there (and possibly additional rabbithole). Not something to simply include here. I will file a ticket about it. |
2 participants
No description provided.