Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed #13363 - apply default signedness to char only #7155

Merged
merged 1 commit into from
Jan 12, 2025

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Jan 1, 2025

No description provided.

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

yes the default sign should only be applied to char

@firewave firewave marked this pull request as ready for review January 9, 2025 17:41
@@ -7185,7 +7185,7 @@ static const Token* parsedecl(const Token* type,
else if (enum_type->isUnsigned())
valuetype->sign = ValueType::Sign::UNSIGNED;
else
valuetype->sign = defaultSignedness;
valuetype->sign = defaultSignedness; // TODO: this is implementation-dependent might be separate from char
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@firewave firewave Jan 11, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

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

@firewave
Copy link
Collaborator Author

well I would have liked if you took care of the enums at once in some reasonable way directly.

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.

@firewave firewave merged commit 93fd884 into danmar:main Jan 12, 2025
60 checks passed
@firewave firewave deleted the default-sign-x branch January 12, 2025 19:21
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