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 all clang -Wsigned-enum-bitfield warnings #2882

Merged
merged 1 commit into from
May 9, 2022

Conversation

seanm
Copy link
Contributor

@seanm seanm commented May 5, 2022

Made enums involved in bitfields unsigned by specifying their underlying type as unsigned char.

Due to a bug, when specifying an underlying type, gcc < 9.3 warns about bitfields not being big enough to hold the enum, even though they are. So keep the plain enum for old gcc.

An example of the bug is here:

https://godbolt.org/z/58aEv8zEq

include/fmt/core.h Outdated Show resolved Hide resolved
@seanm seanm force-pushed the Wsigned-enum-bitfield-fix branch from 606b63b to c92da16 Compare May 5, 2022 18:59
@@ -2029,14 +2029,20 @@ template <typename Context> class basic_format_args {
// between clang and gcc on ARM (#1919).
using format_args = basic_format_args<format_context>;

// We cannot use enum classes as bit fields because of a gcc bug
// We cannot use enum classes as bit fields because of a gcc bug before 9.3
Copy link
Contributor

@vitaut vitaut May 8, 2022

Choose a reason for hiding this comment

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

The issue with pre-9.3 gcc version seems to be about underlying types rather than enum classes because we don't use the former in both branches below. So the part about 9.3 should probably not be folded in the original comment which belongs in front of namespace align to explain why we use namespace + enum instead of scoped enums.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not so sure they are different bugs in fact. But in any case I've improved both the comments in the code and the commit message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@vitaut
Copy link
Contributor

vitaut commented May 8, 2022

Could you provide a repro (ideally a gobolt link) for future reference.

@seanm
Copy link
Contributor Author

seanm commented May 8, 2022

Could you provide a repro (ideally a gobolt link) for future reference.

There's one in the previous PR: #2801 (comment) here: https://godbolt.org/z/58aEv8zEq

Or do you mean to add that link in the code comments?

@vitaut
Copy link
Contributor

vitaut commented May 8, 2022

Or do you mean to add that link in the code comments?

No, I meant in the description of this PR so that it contains all the relevant info.

Made enums involved in bitfields unsigned by specifying their underlying type as unsigned char.

Due to a bug, when specifying an underlying type, gcc < 9.3 warns about bitfields not being big enough to hold the enum, even though they are. So keep the plain enum for old gcc.

An example of the bug is here:

https://godbolt.org/z/58aEv8zEq
@seanm seanm force-pushed the Wsigned-enum-bitfield-fix branch from c92da16 to 6dfb3c3 Compare May 9, 2022 17:52
@vitaut vitaut merged commit f63afd1 into fmtlib:master May 9, 2022
@vitaut
Copy link
Contributor

vitaut commented May 9, 2022

Merged, thank you!

@seanm
Copy link
Contributor Author

seanm commented May 18, 2022

@vitaut any chance there is a release / tag coming soon? These warnings I fixed I'd like to integrate into another project I work on, but they prefer to reference official tags / releases of their upstream projects (like fmtlib)...

@vitaut
Copy link
Contributor

vitaut commented May 21, 2022

Yes by some definition of "soon". I don't have any specific dates though.

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