* [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. @ 2017-10-16 11:20 Sam van Kampen via gcc-patches 2017-10-16 13:26 ` Sam van Kampen via gcc-patches 0 siblings, 1 reply; 16+ messages in thread From: Sam van Kampen via gcc-patches @ 2017-10-16 11:20 UTC (permalink / raw) To: gcc-patches This patch adds a warning flag for the warning described in bug report #61414. My proposed warning flag is -Wenum-bitfield-conversion, which corresponds with the warning flag that clang has for a similar warning. 2017-10-16 Sam van Kampen <sam@segfault.party> * c-family/c.opt: Add a warning flag for struct bit-fields being too small to hold enumerated types. * cp/class.c: Likewise. * doc/invoke.texi: Add documentation for said warning flag. Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt (revision 253769) +++ gcc/c-family/c.opt (working copy) @@ -500,6 +500,10 @@ Wenum-compare C ObjC C++ ObjC++ Var(warn_enum_compare) Init(-1) Warning LangEnabledBy(C ObjC,Wall || Wc++-compat) Warn about comparison of different enum types. +Wenum-bitfield-conversion +C++ Var(warn_enum_bitfield_conversion) Init(1) Warning +Warn about struct bit-fields being too small to hold enumerated types. + Werror C ObjC C++ ObjC++ ; Documented in common.opt Index: gcc/cp/class.c =================================================================== --- gcc/cp/class.c (revision 253769) +++ gcc/cp/class.c (working copy) @@ -3278,10 +3278,11 @@ check_bitfield_decl (tree field) && tree_int_cst_lt (TYPE_SIZE (type), w))) warning_at (DECL_SOURCE_LOCATION (field), 0, "width of %qD exceeds its type", field); - else if (TREE_CODE (type) == ENUMERAL_TYPE + else if (warn_enum_bitfield_conversion + && TREE_CODE (type) == ENUMERAL_TYPE && (0 > (compare_tree_int (w, TYPE_PRECISION (ENUM_UNDERLYING_TYPE (type)))))) - warning_at (DECL_SOURCE_LOCATION (field), 0, + warning_at (DECL_SOURCE_LOCATION (field), OPT_Wenum_bitfield_conversion, "%qD is too small to hold all values of %q#T", field, type); } Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 253769) +++ gcc/doc/invoke.texi (working copy) @@ -277,8 +277,8 @@ Objective-C and Objective-C++ Dialects}. -Wno-discarded-qualifiers -Wno-discarded-array-qualifiers @gol -Wno-div-by-zero -Wdouble-promotion @gol -Wduplicated-branches -Wduplicated-cond @gol --Wempty-body -Wenum-compare -Wno-endif-labels -Wexpansion-to-defined @gol --Werror -Werror=* -Wextra-semi -Wfatal-errors @gol +-Wempty-body -Wenum-bitfield-conversion -Wenum-compare -Wno-endif-labels @gol +-Wexpansion-to-defined -Werror -Werror=* -Wextra-semi -Wfatal-errors @gol -Wfloat-equal -Wformat -Wformat=2 @gol -Wno-format-contains-nul -Wno-format-extra-args @gol -Wformat-nonliteral -Wformat-overflow=@var{n} @gol @@ -6126,6 +6126,12 @@ Warn when an expression is casted to its own type. Warn if an empty body occurs in an @code{if}, @code{else} or @code{do while} statement. This warning is also enabled by @option{-Wextra}. +@item -Wenum-bitfield-conversion +@opindex Wenum-bitfield-conversion +@opindex Wno-enum-bitfield-conversion +Warn about a bit-field potentially being too small to hold all values +of an enumerated type. This warning is enabled by default. + @item -Wenum-compare @opindex Wenum-compare @opindex Wno-enum-compare ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. 2017-10-16 11:20 [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414 Sam van Kampen via gcc-patches @ 2017-10-16 13:26 ` Sam van Kampen via gcc-patches 2017-10-17 0:17 ` Joseph Myers 2017-10-17 7:14 ` Martin Sebor 0 siblings, 2 replies; 16+ messages in thread From: Sam van Kampen via gcc-patches @ 2017-10-16 13:26 UTC (permalink / raw) To: Sam van Kampen; +Cc: gcc-patches ..I just realised that the clang flag is -Wbitfield-enum-conversion, not -Wenum-bitfield-conversion. Please apply the patch below instead, which has replaced the two words to remove the inconsistency. 2017-10-16 Sam van Kampen <sam@segfault.party> * c-family/c.opt: Add a warning flag for struct bit-fields being too small to hold enumerated types. * cp/class.c: Likewise. * doc/invoke.texi: Add documentation for said warning flag. Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt (revision 253784) +++ gcc/c-family/c.opt (working copy) @@ -346,6 +346,10 @@ Wframe-address C ObjC C++ ObjC++ Var(warn_frame_address) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn when __builtin_frame_address or __builtin_return_address is used unsafely. +Wbitfield-enum-conversion +C++ Var(warn_bitfield_enum_conversion) Init(1) Warning +Warn about struct bit-fields being too small to hold enumerated types. + Wbuiltin-declaration-mismatch C ObjC C++ ObjC++ Var(warn_builtin_declaraion_mismatch) Init(1) Warning Warn when a built-in function is declared with the wrong signature. Index: gcc/cp/class.c =================================================================== --- gcc/cp/class.c (revision 253784) +++ gcc/cp/class.c (working copy) @@ -3278,10 +3278,11 @@ check_bitfield_decl (tree field) && tree_int_cst_lt (TYPE_SIZE (type), w))) warning_at (DECL_SOURCE_LOCATION (field), 0, "width of %qD exceeds its type", field); - else if (TREE_CODE (type) == ENUMERAL_TYPE + else if (warn_bitfield_enum_conversion + && TREE_CODE (type) == ENUMERAL_TYPE && (0 > (compare_tree_int (w, TYPE_PRECISION (ENUM_UNDERLYING_TYPE (type)))))) - warning_at (DECL_SOURCE_LOCATION (field), 0, + warning_at (DECL_SOURCE_LOCATION (field), OPT_Wbitfield_enum_conversion, "%qD is too small to hold all values of %q#T", field, type); } Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 253784) +++ gcc/doc/invoke.texi (working copy) @@ -264,7 +264,7 @@ Objective-C and Objective-C++ Dialects}. -Walloca -Walloca-larger-than=@var{n} @gol -Wno-aggressive-loop-optimizations -Warray-bounds -Warray-bounds=@var{n} @gol -Wno-attributes -Wbool-compare -Wbool-operation @gol --Wno-builtin-declaration-mismatch @gol +-Wbitfield-enum-conversion -Wno-builtin-declaration-mismatch @gol -Wno-builtin-macro-redefined -Wc90-c99-compat -Wc99-c11-compat @gol -Wc++-compat -Wc++11-compat -Wc++14-compat @gol -Wcast-align -Wcast-align=strict -Wcast-qual @gol @@ -5431,6 +5431,12 @@ Incrementing a boolean is invalid in C++17, and de This warning is enabled by @option{-Wall}. +@item -Wbitfield-enum-conversion +@opindex Wbitfield-enum-conversion +@opindex Wno-bitfield-enum-conversion +Warn about a bit-field potentially being too small to hold all values +of an enumerated type. This warning is enabled by default. + @item -Wduplicated-branches @opindex Wno-duplicated-branches @opindex Wduplicated-branches ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. 2017-10-16 13:26 ` Sam van Kampen via gcc-patches @ 2017-10-17 0:17 ` Joseph Myers 2017-10-18 13:59 ` Sam van Kampen via gcc-patches 2017-10-17 7:14 ` Martin Sebor 1 sibling, 1 reply; 16+ messages in thread From: Joseph Myers @ 2017-10-17 0:17 UTC (permalink / raw) To: Sam van Kampen; +Cc: gcc-patches On Mon, 16 Oct 2017, Sam van Kampen via gcc-patches wrote: > +Wbitfield-enum-conversion > +C++ Var(warn_bitfield_enum_conversion) Init(1) Warning > +Warn about struct bit-fields being too small to hold enumerated types. Any option supported for C++ should also be supported for ObjC++ unless there is a clear reason it cannot work for ObjC++. > +@item -Wbitfield-enum-conversion > +@opindex Wbitfield-enum-conversion > +@opindex Wno-bitfield-enum-conversion > +Warn about a bit-field potentially being too small to hold all values > +of an enumerated type. This warning is enabled by default. The documentation of the option also needs to indicate that it's for C++/ObjC++ only, similar to other such options. The patch also needs to add a testcase to the testsuite that verifies the warnings issues in appropriate cases (and that warnings are not issued when the bit-field is large enough). -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. 2017-10-17 0:17 ` Joseph Myers @ 2017-10-18 13:59 ` Sam van Kampen via gcc-patches 0 siblings, 0 replies; 16+ messages in thread From: Sam van Kampen via gcc-patches @ 2017-10-18 13:59 UTC (permalink / raw) To: Joseph Myers; +Cc: gcc-patches On Mon, Oct 16, 2017 at 11:31:10PM +0000, Joseph Myers wrote: > On Mon, 16 Oct 2017, Sam van Kampen via gcc-patches wrote: > > > +Wbitfield-enum-conversion > > +C++ Var(warn_bitfield_enum_conversion) Init(1) Warning > > +Warn about struct bit-fields being too small to hold enumerated types. > > Any option supported for C++ should also be supported for ObjC++ unless > there is a clear reason it cannot work for ObjC++. > [...] > The documentation of the option also needs to indicate that it's for > C++/ObjC++ only, similar to other such options. I've added the ObjC++ flag to the warning declaration above and I've expanded the documentation to note that it only applies to C++/ObjC++. > The patch also needs to add a testcase to the testsuite that verifies the > warnings issues in appropriate cases (and that warnings are not issued > when the bit-field is large enough). I've added such a testcase as well. I'll send the updated version of the patch, I'm just waiting to hear back from Martin on whether the flag should fall under -Wextra or fall under no flag and be disabled unless explicitly specified. > -- > Joseph S. Myers > joseph@codesourcery.com Thanks for the feedback, Sam ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. 2017-10-16 13:26 ` Sam van Kampen via gcc-patches 2017-10-17 0:17 ` Joseph Myers @ 2017-10-17 7:14 ` Martin Sebor 2017-10-18 13:54 ` Sam van Kampen via gcc-patches 2017-10-19 19:22 ` [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414 Eric Gallager 1 sibling, 2 replies; 16+ messages in thread From: Martin Sebor @ 2017-10-17 7:14 UTC (permalink / raw) To: Sam van Kampen; +Cc: gcc-patches On 10/16/2017 06:37 AM, Sam van Kampen via gcc-patches wrote: > ..I just realised that the clang flag is -Wbitfield-enum-conversion, not > -Wenum-bitfield-conversion. Please apply the patch below instead, which > has replaced the two words to remove the inconsistency. > > 2017-10-16 Sam van Kampen <sam@segfault.party> > > * c-family/c.opt: Add a warning flag for struct bit-fields > being too small to hold enumerated types. > * cp/class.c: Likewise. > * doc/invoke.texi: Add documentation for said warning flag. > > Index: gcc/c-family/c.opt > =================================================================== > --- gcc/c-family/c.opt (revision 253784) > +++ gcc/c-family/c.opt (working copy) > @@ -346,6 +346,10 @@ Wframe-address > C ObjC C++ ObjC++ Var(warn_frame_address) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) > Warn when __builtin_frame_address or __builtin_return_address is used unsafely. > > +Wbitfield-enum-conversion > +C++ Var(warn_bitfield_enum_conversion) Init(1) Warning > +Warn about struct bit-fields being too small to hold enumerated types. Warning by default is usually reserved for problems that are most likely indicative of bugs. Does this rise to that level? (It seems that it should be in the same class as -Wconversion). > + > Wbuiltin-declaration-mismatch > C ObjC C++ ObjC++ Var(warn_builtin_declaraion_mismatch) Init(1) Warning > Warn when a built-in function is declared with the wrong signature. > Index: gcc/cp/class.c > =================================================================== > --- gcc/cp/class.c (revision 253784) > +++ gcc/cp/class.c (working copy) > @@ -3278,10 +3278,11 @@ check_bitfield_decl (tree field) > && tree_int_cst_lt (TYPE_SIZE (type), w))) > warning_at (DECL_SOURCE_LOCATION (field), 0, > "width of %qD exceeds its type", field); > - else if (TREE_CODE (type) == ENUMERAL_TYPE > + else if (warn_bitfield_enum_conversion > + && TREE_CODE (type) == ENUMERAL_TYPE > && (0 > (compare_tree_int > (w, TYPE_PRECISION (ENUM_UNDERLYING_TYPE (type)))))) > - warning_at (DECL_SOURCE_LOCATION (field), 0, > + warning_at (DECL_SOURCE_LOCATION (field), OPT_Wbitfield_enum_conversion, > "%qD is too small to hold all values of %q#T", > field, type); I would suggest to include a bit more detail in the message, such as the smallest number of bits needed to represent every value of the enumeration. It's also often helpful to include a note pointing to the relevant declaration (in this case it could be the bit-field). > } > Index: gcc/doc/invoke.texi > =================================================================== > --- gcc/doc/invoke.texi (revision 253784) > +++ gcc/doc/invoke.texi (working copy) > @@ -264,7 +264,7 @@ Objective-C and Objective-C++ Dialects}. > -Walloca -Walloca-larger-than=@var{n} @gol > -Wno-aggressive-loop-optimizations -Warray-bounds -Warray-bounds=@var{n} @gol > -Wno-attributes -Wbool-compare -Wbool-operation @gol > --Wno-builtin-declaration-mismatch @gol > +-Wbitfield-enum-conversion -Wno-builtin-declaration-mismatch @gol > -Wno-builtin-macro-redefined -Wc90-c99-compat -Wc99-c11-compat @gol > -Wc++-compat -Wc++11-compat -Wc++14-compat @gol > -Wcast-align -Wcast-align=strict -Wcast-qual @gol > @@ -5431,6 +5431,12 @@ Incrementing a boolean is invalid in C++17, and de > > This warning is enabled by @option{-Wall}. > > +@item -Wbitfield-enum-conversion > +@opindex Wbitfield-enum-conversion > +@opindex Wno-bitfield-enum-conversion > +Warn about a bit-field potentially being too small to hold all values > +of an enumerated type. This warning is enabled by default. > + The brief description makes me wonder what this really means. What is the meaning of "potentially?" What (what expressions) triggers the warning? Presumably it's initialization and assignment. Does explicit or implicit conversion also trigger is? I would suggest to expand the documentation to obviate these questions, and perhaps also include an example. Martin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. 2017-10-17 7:14 ` Martin Sebor @ 2017-10-18 13:54 ` Sam van Kampen via gcc-patches 2017-10-18 16:00 ` Martin Sebor 2017-10-19 19:22 ` [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414 Eric Gallager 1 sibling, 1 reply; 16+ messages in thread From: Sam van Kampen via gcc-patches @ 2017-10-18 13:54 UTC (permalink / raw) To: Martin Sebor; +Cc: gcc-patches On Mon, Oct 16, 2017 at 08:56:05PM -0600, Martin Sebor wrote: > On 10/16/2017 06:37 AM, Sam van Kampen via gcc-patches wrote: > > ..I just realised that the clang flag is -Wbitfield-enum-conversion, not > > -Wenum-bitfield-conversion. Please apply the patch below instead, which > > has replaced the two words to remove the inconsistency. > > > > 2017-10-16 Sam van Kampen <sam@segfault.party> > > > > * c-family/c.opt: Add a warning flag for struct bit-fields > > being too small to hold enumerated types. > > * cp/class.c: Likewise. > > * doc/invoke.texi: Add documentation for said warning flag. > > > > Index: gcc/c-family/c.opt > > =================================================================== > > --- gcc/c-family/c.opt (revision 253784) > > +++ gcc/c-family/c.opt (working copy) > > @@ -346,6 +346,10 @@ Wframe-address > > C ObjC C++ ObjC++ Var(warn_frame_address) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) > > Warn when __builtin_frame_address or __builtin_return_address is used unsafely. > > > > +Wbitfield-enum-conversion > > +C++ Var(warn_bitfield_enum_conversion) Init(1) Warning > > +Warn about struct bit-fields being too small to hold enumerated types. > > Warning by default is usually reserved for problems that are > most likely indicative of bugs. Does this rise to that level? > (It seems that it should be in the same class as -Wconversion). > Fair enough, I didn't know whether to change the way it currently was triggered. Do you think it should fall under -Wextra (I don't think it falls under -Wall, since it isn't "easy to avoid or modify to prevent the warning" because it may be valid and wanted behavior), or should it be enabled by no other flag? > > + > > Wbuiltin-declaration-mismatch > > C ObjC C++ ObjC++ Var(warn_builtin_declaraion_mismatch) Init(1) Warning > > Warn when a built-in function is declared with the wrong signature. > > Index: gcc/cp/class.c > > =================================================================== > > --- gcc/cp/class.c (revision 253784) > > +++ gcc/cp/class.c (working copy) > > @@ -3278,10 +3278,11 @@ check_bitfield_decl (tree field) > > && tree_int_cst_lt (TYPE_SIZE (type), w))) > > warning_at (DECL_SOURCE_LOCATION (field), 0, > > "width of %qD exceeds its type", field); > > - else if (TREE_CODE (type) == ENUMERAL_TYPE > > + else if (warn_bitfield_enum_conversion > > + && TREE_CODE (type) == ENUMERAL_TYPE > > && (0 > (compare_tree_int > > (w, TYPE_PRECISION (ENUM_UNDERLYING_TYPE (type)))))) > > - warning_at (DECL_SOURCE_LOCATION (field), 0, > > + warning_at (DECL_SOURCE_LOCATION (field), OPT_Wbitfield_enum_conversion, > > "%qD is too small to hold all values of %q#T", > > field, type); > > I would suggest to include a bit more detail in the message, such > as the smallest number of bits needed to represent every value of > the enumeration. It's also often helpful to include a note > pointing to the relevant declaration (in this case it could be > the bit-field). > The warning points to the bit-field ('field:2' is too small to hold...), but I've added a note in an updated patch that tells the user what the precision of the enum's underlying type is. > > } > > Index: gcc/doc/invoke.texi > > =================================================================== > > --- gcc/doc/invoke.texi (revision 253784) > > +++ gcc/doc/invoke.texi (working copy) > > @@ -264,7 +264,7 @@ Objective-C and Objective-C++ Dialects}. > > -Walloca -Walloca-larger-than=@var{n} @gol > > -Wno-aggressive-loop-optimizations -Warray-bounds -Warray-bounds=@var{n} @gol > > -Wno-attributes -Wbool-compare -Wbool-operation @gol > > --Wno-builtin-declaration-mismatch @gol > > +-Wbitfield-enum-conversion -Wno-builtin-declaration-mismatch @gol > > -Wno-builtin-macro-redefined -Wc90-c99-compat -Wc99-c11-compat @gol > > -Wc++-compat -Wc++11-compat -Wc++14-compat @gol > > -Wcast-align -Wcast-align=strict -Wcast-qual @gol > > @@ -5431,6 +5431,12 @@ Incrementing a boolean is invalid in C++17, and de > > > > This warning is enabled by @option{-Wall}. > > > > +@item -Wbitfield-enum-conversion > > +@opindex Wbitfield-enum-conversion > > +@opindex Wno-bitfield-enum-conversion > > +Warn about a bit-field potentially being too small to hold all values > > +of an enumerated type. This warning is enabled by default. > > + > > The brief description makes me wonder what this really means. What > is the meaning of "potentially?" What (what expressions) triggers > the warning? Presumably it's initialization and assignment. Does > explicit or implicit conversion also trigger is? I would suggest > to expand the documentation to obviate these questions, and perhaps > also include an example. > I've changed the description in both c.opt and invoke.texi to be more specific, it now notes that it warns when 1. An enum in a bit-field is a scoped enum... 2. ...which has an underlying type which has a higher precision than the width of the bit-field. > Martin Thanks for the feedback, Sam ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. 2017-10-18 13:54 ` Sam van Kampen via gcc-patches @ 2017-10-18 16:00 ` Martin Sebor 2017-10-18 19:29 ` Sam van Kampen via gcc-patches 0 siblings, 1 reply; 16+ messages in thread From: Martin Sebor @ 2017-10-18 16:00 UTC (permalink / raw) To: Sam van Kampen; +Cc: gcc-patches > Fair enough, I didn't know whether to change the way it currently was > triggered. Do you think it should fall under -Wextra (I don't think it > falls under -Wall, since it isn't "easy to avoid or modify to prevent > the warning" because it may be valid and wanted behavior), or should it > be enabled by no other flag? I think it depends on the implementation of the warning. With the current (fairly restrictive) behavior I'd say it should be disabled by default. But if it were to be changed to more closely match the Clang behavior and only warn for bit-field declarations that cannot represent all enumerators of the enumerated type, then including it in -Wall would seem helpful to me. I.e., Clang doesn't warn on this and IIUC that's what the reporter of the bug also expects: enum E: unsigned { E3 = 15 }; struct S { E i: 4; }; (There is value in warning on this as well, but I think most users will not be interested in it, so making the warning a two-level one where level 1 warns same as Clang and level 2 same as GCC does now might give us the best of both worlds). > The warning points to the bit-field ('field:2' is too small to hold...), > but I've added a note in an updated patch that tells the user what the > precision of the enum's underlying type is. That will be useful, thanks! > I've changed the description in both c.opt and invoke.texi to be more > specific, it now notes that it warns when > 1. An enum in a bit-field is a scoped enum... > 2. ...which has an underlying type which has a higher precision > than the width of the bit-field. Sounds good. Martin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. 2017-10-18 16:00 ` Martin Sebor @ 2017-10-18 19:29 ` Sam van Kampen via gcc-patches 2017-10-19 3:23 ` Martin Sebor 2017-10-20 16:05 ` Jason Merrill 0 siblings, 2 replies; 16+ messages in thread From: Sam van Kampen via gcc-patches @ 2017-10-18 19:29 UTC (permalink / raw) To: Martin Sebor; +Cc: gcc-patches On Wed, Oct 18, 2017 at 09:46:08AM -0600, Martin Sebor wrote: > > Fair enough, I didn't know whether to change the way it currently was > > triggered. Do you think it should fall under -Wextra (I don't think it > > falls under -Wall, since it isn't "easy to avoid or modify to prevent > > the warning" because it may be valid and wanted behavior), or should it > > be enabled by no other flag? > > I think it depends on the implementation of the warning. With > the current (fairly restrictive) behavior I'd say it should be > disabled by default. But if it were to be changed to more closely > match the Clang behavior and only warn for bit-field declarations > that cannot represent all enumerators of the enumerated type, then > including it in -Wall would seem helpful to me. > > I.e., Clang doesn't warn on this and IIUC that's what the reporter > of the bug also expects: > > enum E: unsigned { E3 = 15 }; > > struct S { E i: 4; }; > > (There is value in warning on this as well, but I think most users > will not be interested in it, so making the warning a two-level one > where level 1 warns same as Clang and level 2 same as GCC does now > might give us the best of both worlds). I see what you mean - that is the behavior I wanted to implement in the first place, but Jonathan Wakely rightly pointed out that when an enumeration is scoped, all values of its underlying type are valid enumeration values, and so the bit-field you declare in 'S' _is_ too small to hold all values of 'enum E'. Here's the corresponding text from draft N4659 of the C++17 standard, §10.2/8 [dcl.enum] For an enumeration whose underlying type is fixed, the values of the enumeration are the values of the underlying type. [...] It is possible to define an enumeration that has values not defined by any of its enumerators. Still, warning when a bit-field can't hold all enumerators instead of all values may be a good idea. I've looked into it, and it does require recalculating the maximum and minimum enumerator value, since the bounds of the underlying type are saved in TYPE_MIN_VALUE and TYPE_MAX_VALUE when the enumeration is scoped, instead of the min/max enumerator value. Would adding that separate warning level be part of a separate patch, or should I add it to this one? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. 2017-10-18 19:29 ` Sam van Kampen via gcc-patches @ 2017-10-19 3:23 ` Martin Sebor 2017-10-20 16:05 ` Jason Merrill 1 sibling, 0 replies; 16+ messages in thread From: Martin Sebor @ 2017-10-19 3:23 UTC (permalink / raw) To: Sam van Kampen; +Cc: gcc-patches On 10/18/2017 01:15 PM, Sam van Kampen wrote: > On Wed, Oct 18, 2017 at 09:46:08AM -0600, Martin Sebor wrote: >>> Fair enough, I didn't know whether to change the way it currently was >>> triggered. Do you think it should fall under -Wextra (I don't think it >>> falls under -Wall, since it isn't "easy to avoid or modify to prevent >>> the warning" because it may be valid and wanted behavior), or should it >>> be enabled by no other flag? >> >> I think it depends on the implementation of the warning. With >> the current (fairly restrictive) behavior I'd say it should be >> disabled by default. But if it were to be changed to more closely >> match the Clang behavior and only warn for bit-field declarations >> that cannot represent all enumerators of the enumerated type, then >> including it in -Wall would seem helpful to me. >> >> I.e., Clang doesn't warn on this and IIUC that's what the reporter >> of the bug also expects: >> >> enum E: unsigned { E3 = 15 }; >> >> struct S { E i: 4; }; >> >> (There is value in warning on this as well, but I think most users >> will not be interested in it, so making the warning a two-level one >> where level 1 warns same as Clang and level 2 same as GCC does now >> might give us the best of both worlds). > > I see what you mean - that is the behavior I wanted to implement in the > first place, but Jonathan Wakely rightly pointed out that when an > enumeration is scoped, all values of its underlying type are valid > enumeration values, and so the bit-field you declare in 'S' _is_ too > small to hold all values of 'enum E'. > > Here's the corresponding text from draft N4659 of the C++17 standard, > §10.2/8 [dcl.enum] > > For an enumeration whose underlying type is fixed, the values of the > enumeration are the values of the underlying type. [...] It is possible > to define an enumeration that has values not defined by any of its > enumerators. > > Still, warning when a bit-field can't hold all enumerators instead of > all values may be a good idea. I've looked into it, and it does require > recalculating the maximum and minimum enumerator value, since the bounds > of the underlying type are saved in TYPE_MIN_VALUE and TYPE_MAX_VALUE > when the enumeration is scoped, instead of the min/max enumerator value. > > Would adding that separate warning level be part of a separate patch, or > should I add it to this one? I think it makes sense to submit the feature it in its final form, whatever you decide that will be. Martin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. 2017-10-18 19:29 ` Sam van Kampen via gcc-patches 2017-10-19 3:23 ` Martin Sebor @ 2017-10-20 16:05 ` Jason Merrill 2019-11-25 14:40 ` Jakub Jelinek 1 sibling, 1 reply; 16+ messages in thread From: Jason Merrill @ 2017-10-20 16:05 UTC (permalink / raw) To: Sam van Kampen; +Cc: Martin Sebor, gcc-patches List On Wed, Oct 18, 2017 at 3:15 PM, Sam van Kampen via gcc-patches <gcc-patches@gcc.gnu.org> wrote: > On Wed, Oct 18, 2017 at 09:46:08AM -0600, Martin Sebor wrote: >> > Fair enough, I didn't know whether to change the way it currently was >> > triggered. Do you think it should fall under -Wextra (I don't think it >> > falls under -Wall, since it isn't "easy to avoid or modify to prevent >> > the warning" because it may be valid and wanted behavior), or should it >> > be enabled by no other flag? >> >> I think it depends on the implementation of the warning. With >> the current (fairly restrictive) behavior I'd say it should be >> disabled by default. But if it were to be changed to more closely >> match the Clang behavior and only warn for bit-field declarations >> that cannot represent all enumerators of the enumerated type, then >> including it in -Wall would seem helpful to me. >> >> I.e., Clang doesn't warn on this and IIUC that's what the reporter >> of the bug also expects: >> >> enum E: unsigned { E3 = 15 }; >> >> struct S { E i: 4; }; >> >> (There is value in warning on this as well, but I think most users >> will not be interested in it, so making the warning a two-level one >> where level 1 warns same as Clang and level 2 same as GCC does now >> might give us the best of both worlds). > > I see what you mean - that is the behavior I wanted to implement in the > first place, but Jonathan Wakely rightly pointed out that when an > enumeration is scoped, all values of its underlying type are valid > enumeration values, and so the bit-field you declare in 'S' _is_ too > small to hold all values of 'enum E'. > > Here's the corresponding text from draft N4659 of the C++17 standard, > §10.2/8 [dcl.enum] > > For an enumeration whose underlying type is fixed, the values of the > enumeration are the values of the underlying type. [...] It is possible > to define an enumeration that has values not defined by any of its > enumerators. > > Still, warning when a bit-field can't hold all enumerators instead of > all values may be a good idea. I've looked into it, and it does require > recalculating the maximum and minimum enumerator value, since the bounds > of the underlying type are saved in TYPE_MIN_VALUE and TYPE_MAX_VALUE > when the enumeration is scoped, instead of the min/max enumerator value. > > Would adding that separate warning level be part of a separate patch, or > should I add it to this one? I think making that behavior the default would be appropriate. The current behavior for scoped enums seems like a bug; sure, all values of the underlying type are valid, but that's also true for "unsigned i : 4", and we don't warn about that. Jason ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. 2017-10-20 16:05 ` Jason Merrill @ 2019-11-25 14:40 ` Jakub Jelinek 2019-11-25 14:54 ` Jakub Jelinek 0 siblings, 1 reply; 16+ messages in thread From: Jakub Jelinek @ 2019-11-25 14:40 UTC (permalink / raw) To: Jason Merrill; +Cc: Sam van Kampen, Martin Sebor, gcc-patches List On Fri, Oct 20, 2017 at 11:59:39AM -0400, Jason Merrill wrote: > > Still, warning when a bit-field can't hold all enumerators instead of > > all values may be a good idea. I've looked into it, and it does require > > recalculating the maximum and minimum enumerator value, since the bounds > > of the underlying type are saved in TYPE_MIN_VALUE and TYPE_MAX_VALUE > > when the enumeration is scoped, instead of the min/max enumerator value. > > > > Would adding that separate warning level be part of a separate patch, or > > should I add it to this one? > > I think making that behavior the default would be appropriate. The > current behavior for scoped enums seems like a bug; sure, all values > of the underlying type are valid, but that's also true for "unsigned i > : 4", and we don't warn about that. Note, the chosen switch name -Wbitfield-enum-conversion looks wrong to me, that is a (non-default) warning option that clang implements, but it actually is completely different warning, which GCC doesn't implement and the warning GCC has is something clang on the other side doesn't implement. So reusing the same option name for something different is bad. The warning clang++ implements is warning when storing a value with enum type into a bitfield with implicit cast, if the number of bitfield bits (the bitfield could very well be just int x : 2; or unsigned y : 3; etc.) is smaller than what is needed for the range of the enum. So, if we want to add a warning option for this warning, it should be something like -Wbitfield-enum-width or so. Agreed on that we just should only consider the range of enum values and not the underlying type for the warning, whether the warning has a warning switch or not. I think the reason why for C++ the warning is implemented by checking the precision of the underlying type is that this way was the cheapest, at least when scoped enums or enums with fixed underlying type or mode attribute isn't used, then finish_enum_value_list already computed the needed precision. While e.g. in the C FE the same warning is implemented by remembering the minimum and maximum values in type_lang_specific (although, that seems like a waste, why it doesn't just store the minimum precision instead of the two trees). finish_enum_value_list already computes almost everything even when using fixed underlying type or mode attribute, except the signop sgn = tree_int_cst_sgn (minnode) >= 0 ? UNSIGNED : SIGNED; int lowprec = tree_int_cst_min_precision (minnode, sgn); int highprec = tree_int_cst_min_precision (maxnode, sgn); int precision = MAX (lowprec, highprec); I guess the question is, shall we store the minimum precision needed somewhere by finish_enum_value_list (perhaps only bother if the precision of the underlying type is not the same) or compute it each time again (which would mean for each bitfield of enum type walk the list of all the enum values)? And, if we should store it somewhere, any preferences where? These days C++ FE has TYPE_LANG_SPECIFIC just for classes, so options are just use the class TYPE_LANG_SPECIFIC and just stick the min/max values in some trees in there, restore having different lang specific variants and store just minimum precision in there, add hash map from tree (hashed using TYPE_UID) to precision, something else? Jakub ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. 2019-11-25 14:40 ` Jakub Jelinek @ 2019-11-25 14:54 ` Jakub Jelinek 2019-11-25 23:42 ` [C++ PATCH] Fix up is too small to hold all values of enum warning (PR c++/61414) Jakub Jelinek 0 siblings, 1 reply; 16+ messages in thread From: Jakub Jelinek @ 2019-11-25 14:54 UTC (permalink / raw) To: Jason Merrill; +Cc: Sam van Kampen, Martin Sebor, gcc-patches List On Mon, Nov 25, 2019 at 03:39:32PM +0100, Jakub Jelinek wrote: > I guess the question is, shall we store the minimum precision needed > somewhere by finish_enum_value_list (perhaps only bother if the precision > of the underlying type is not the same) or compute it each time again > (which would mean for each bitfield of enum type walk the list of all the > enum values)? And, if we should store it somewhere, any preferences where? > These days C++ FE has TYPE_LANG_SPECIFIC just for classes, so options are > just use the class TYPE_LANG_SPECIFIC and just stick the min/max values in > some trees in there, restore having different lang specific variants and > store just minimum precision in there, add hash map from tree (hashed using > TYPE_UID) to precision, something else? Or yet another possibility, only compute it on demand and use a hash map as cache. Jakub ^ permalink raw reply [flat|nested] 16+ messages in thread
* [C++ PATCH] Fix up is too small to hold all values of enum warning (PR c++/61414) 2019-11-25 14:54 ` Jakub Jelinek @ 2019-11-25 23:42 ` Jakub Jelinek 2019-11-26 21:52 ` Jason Merrill 0 siblings, 1 reply; 16+ messages in thread From: Jakub Jelinek @ 2019-11-25 23:42 UTC (permalink / raw) To: Jason Merrill; +Cc: Sam van Kampen, Martin Sebor, gcc-patches List Hi! On Mon, Nov 25, 2019 at 03:46:23PM +0100, Jakub Jelinek wrote: > On Mon, Nov 25, 2019 at 03:39:32PM +0100, Jakub Jelinek wrote: > > I guess the question is, shall we store the minimum precision needed > > somewhere by finish_enum_value_list (perhaps only bother if the precision > > of the underlying type is not the same) or compute it each time again > > (which would mean for each bitfield of enum type walk the list of all the > > enum values)? And, if we should store it somewhere, any preferences where? > > These days C++ FE has TYPE_LANG_SPECIFIC just for classes, so options are > > just use the class TYPE_LANG_SPECIFIC and just stick the min/max values in > > some trees in there, restore having different lang specific variants and > > store just minimum precision in there, add hash map from tree (hashed using > > TYPE_UID) to precision, something else? > > Or yet another possibility, only compute it on demand and use a hash map as > cache. Here is that option implemented, bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2019-11-26 Jakub Jelinek <jakub@redhat.com> PR c++/61414 * c-attribs.c (handle_mode_attribute): Add mode attribute to ENUMERAL_TYPEs. * class.c (enum_to_min_precision): New hash_map. (enum_min_precision): New function. (check_bitfield_decl): Use it. * g++.dg/cpp0x/enum23.C: Remove xfail. * g++.dg/cpp0x/enum28.C: New test. --- gcc/c-family/c-attribs.c.jj 2019-11-23 11:05:50.813492164 +0100 +++ gcc/c-family/c-attribs.c 2019-11-25 16:47:35.317750531 +0100 @@ -1866,6 +1866,7 @@ handle_mode_attribute (tree *node, tree typefm = make_signed_type (TYPE_PRECISION (typefm)); TREE_TYPE (typefm) = type; } + *no_add_attrs = false; } else if (VECTOR_MODE_P (mode) ? TREE_CODE (type) != TREE_CODE (TREE_TYPE (typefm)) --- gcc/cp/class.c.jj 2019-11-16 18:13:42.844606979 +0100 +++ gcc/cp/class.c 2019-11-25 16:42:15.050633229 +0100 @@ -3265,6 +3265,60 @@ add_implicitly_declared_members (tree t, } } +/* Cache of enum_min_precision values. */ +static GTY((deletable)) hash_map<tree, int> *enum_to_min_precision; + +/* Return the minimum precision of a bit-field needed to store all + enumerators of ENUMERAL_TYPE TYPE. */ + +static int +enum_min_precision (tree type) +{ + type = TYPE_MAIN_VARIANT (type); + /* For unscoped enums without fixed underlying type and without mode + attribute we can just use precision of the underlying type. */ + if (UNSCOPED_ENUM_P (type) + && !ENUM_FIXED_UNDERLYING_TYPE_P (type) + && !lookup_attribute ("mode", TYPE_ATTRIBUTES (type))) + return TYPE_PRECISION (ENUM_UNDERLYING_TYPE (type)); + + if (enum_to_min_precision == NULL) + enum_to_min_precision = hash_map<tree, int>::create_ggc (37); + + bool existed; + int prec = enum_to_min_precision->get_or_insert (type, &existed); + if (existed) + return prec; + + tree minnode, maxnode; + if (TYPE_VALUES (type)) + { + minnode = maxnode = NULL_TREE; + for (tree values = TYPE_VALUES (type); + values; values = TREE_CHAIN (values)) + { + tree decl = TREE_VALUE (values); + tree value = DECL_INITIAL (decl); + if (value == error_mark_node) + value = integer_zero_node; + if (!minnode) + minnode = maxnode = value; + else if (tree_int_cst_lt (maxnode, value)) + maxnode = value; + else if (tree_int_cst_lt (value, minnode)) + minnode = value; + } + } + else + minnode = maxnode = integer_zero_node; + + signop sgn = tree_int_cst_sgn (minnode) >= 0 ? UNSIGNED : SIGNED; + int lowprec = tree_int_cst_min_precision (minnode, sgn); + int highprec = tree_int_cst_min_precision (maxnode, sgn); + prec = MAX (lowprec, highprec); + return prec; +} + /* FIELD is a bit-field. We are finishing the processing for its enclosing type. Issue any appropriate messages and set appropriate flags. Returns false if an error has been diagnosed. */ @@ -3326,7 +3380,7 @@ check_bitfield_decl (tree field) "width of %qD exceeds its type", field); else if (TREE_CODE (type) == ENUMERAL_TYPE) { - int prec = TYPE_PRECISION (ENUM_UNDERLYING_TYPE (type)); + int prec = enum_min_precision (type); if (compare_tree_int (w, prec) < 0) warning_at (DECL_SOURCE_LOCATION (field), 0, "%qD is too small to hold all values of %q#T", --- gcc/testsuite/g++.dg/cpp0x/enum23.C.jj 2013-02-16 09:31:06.400506406 +0100 +++ gcc/testsuite/g++.dg/cpp0x/enum23.C 2019-11-25 16:51:19.532332223 +0100 @@ -5,5 +5,5 @@ enum class MyEnum { A = 1 }; struct MyClass { - MyEnum Field1 : 3; // { dg-bogus "warning: 'MyClass::Field1' is too small" "" { xfail *-*-* } } + MyEnum Field1 : 3; // { dg-bogus "warning: 'MyClass::Field1' is too small" } }; --- gcc/testsuite/g++.dg/cpp0x/enum38.C.jj 2019-11-25 16:50:56.323686060 +0100 +++ gcc/testsuite/g++.dg/cpp0x/enum38.C 2019-11-25 16:52:28.466281278 +0100 @@ -0,0 +1,25 @@ +// PR c++/61414 +// { dg-do compile { target c++11 } } + +enum C { C0 = -4, C1 = 3 }; +enum D { D0 = 0, D1 = 15 }; +enum class E { E0 = -4, E1 = 3 }; +enum F : unsigned { F0 = 0, F1 = 15 }; +enum __attribute__((__mode__ (__QI__))) G { G0 = -4, G1 = 3 }; +enum __attribute__((__mode__ (__HI__))) H { H0 = 0, H1 = 15 }; + +struct S +{ + C a : 2; // { dg-warning "'S::a' is too small to hold all values of 'enum C'" } + C b : 3; // { dg-bogus "'S::b' is too small to hold all values of 'enum C'" } + D c : 3; // { dg-warning "'S::c' is too small to hold all values of 'enum D'" } + D d : 4; // { dg-bogus "'S::d' is too small to hold all values of 'enum D'" } + E e : 2; // { dg-warning "'S::e' is too small to hold all values of 'enum class E'" } + E f : 3; // { dg-bogus "'S::f' is too small to hold all values of 'enum class E'" } + F g : 3; // { dg-warning "'S::g' is too small to hold all values of 'enum F'" } + F h : 4; // { dg-bogus "'S::h' is too small to hold all values of 'enum F'" } + G i : 2; // { dg-warning "'S::i' is too small to hold all values of 'enum G'" } + G j : 3; // { dg-bogus "'S::j' is too small to hold all values of 'enum G'" } + H k : 3; // { dg-warning "'S::k' is too small to hold all values of 'enum H'" } + H l : 4; // { dg-bogus "'S::l' is too small to hold all values of 'enum H'" } +}; Jakub ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [C++ PATCH] Fix up is too small to hold all values of enum warning (PR c++/61414) 2019-11-25 23:42 ` [C++ PATCH] Fix up is too small to hold all values of enum warning (PR c++/61414) Jakub Jelinek @ 2019-11-26 21:52 ` Jason Merrill 0 siblings, 0 replies; 16+ messages in thread From: Jason Merrill @ 2019-11-26 21:52 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Sam van Kampen, Martin Sebor, gcc-patches List On 11/25/19 6:32 PM, Jakub Jelinek wrote: > Hi! > > On Mon, Nov 25, 2019 at 03:46:23PM +0100, Jakub Jelinek wrote: >> On Mon, Nov 25, 2019 at 03:39:32PM +0100, Jakub Jelinek wrote: >>> I guess the question is, shall we store the minimum precision needed >>> somewhere by finish_enum_value_list (perhaps only bother if the precision >>> of the underlying type is not the same) or compute it each time again >>> (which would mean for each bitfield of enum type walk the list of all the >>> enum values)? And, if we should store it somewhere, any preferences where? >>> These days C++ FE has TYPE_LANG_SPECIFIC just for classes, so options are >>> just use the class TYPE_LANG_SPECIFIC and just stick the min/max values in >>> some trees in there, restore having different lang specific variants and >>> store just minimum precision in there, add hash map from tree (hashed using >>> TYPE_UID) to precision, something else? >> >> Or yet another possibility, only compute it on demand and use a hash map as >> cache. > > Here is that option implemented, bootstrapped/regtested on x86_64-linux and > i686-linux, ok for trunk? OK, thanks. > 2019-11-26 Jakub Jelinek <jakub@redhat.com> > > PR c++/61414 > * c-attribs.c (handle_mode_attribute): Add mode attribute to > ENUMERAL_TYPEs. > > * class.c (enum_to_min_precision): New hash_map. > (enum_min_precision): New function. > (check_bitfield_decl): Use it. > > * g++.dg/cpp0x/enum23.C: Remove xfail. > * g++.dg/cpp0x/enum28.C: New test. > > --- gcc/c-family/c-attribs.c.jj 2019-11-23 11:05:50.813492164 +0100 > +++ gcc/c-family/c-attribs.c 2019-11-25 16:47:35.317750531 +0100 > @@ -1866,6 +1866,7 @@ handle_mode_attribute (tree *node, tree > typefm = make_signed_type (TYPE_PRECISION (typefm)); > TREE_TYPE (typefm) = type; > } > + *no_add_attrs = false; > } > else if (VECTOR_MODE_P (mode) > ? TREE_CODE (type) != TREE_CODE (TREE_TYPE (typefm)) > --- gcc/cp/class.c.jj 2019-11-16 18:13:42.844606979 +0100 > +++ gcc/cp/class.c 2019-11-25 16:42:15.050633229 +0100 > @@ -3265,6 +3265,60 @@ add_implicitly_declared_members (tree t, > } > } > > +/* Cache of enum_min_precision values. */ > +static GTY((deletable)) hash_map<tree, int> *enum_to_min_precision; > + > +/* Return the minimum precision of a bit-field needed to store all > + enumerators of ENUMERAL_TYPE TYPE. */ > + > +static int > +enum_min_precision (tree type) > +{ > + type = TYPE_MAIN_VARIANT (type); > + /* For unscoped enums without fixed underlying type and without mode > + attribute we can just use precision of the underlying type. */ > + if (UNSCOPED_ENUM_P (type) > + && !ENUM_FIXED_UNDERLYING_TYPE_P (type) > + && !lookup_attribute ("mode", TYPE_ATTRIBUTES (type))) > + return TYPE_PRECISION (ENUM_UNDERLYING_TYPE (type)); > + > + if (enum_to_min_precision == NULL) > + enum_to_min_precision = hash_map<tree, int>::create_ggc (37); > + > + bool existed; > + int prec = enum_to_min_precision->get_or_insert (type, &existed); > + if (existed) > + return prec; > + > + tree minnode, maxnode; > + if (TYPE_VALUES (type)) > + { > + minnode = maxnode = NULL_TREE; > + for (tree values = TYPE_VALUES (type); > + values; values = TREE_CHAIN (values)) > + { > + tree decl = TREE_VALUE (values); > + tree value = DECL_INITIAL (decl); > + if (value == error_mark_node) > + value = integer_zero_node; > + if (!minnode) > + minnode = maxnode = value; > + else if (tree_int_cst_lt (maxnode, value)) > + maxnode = value; > + else if (tree_int_cst_lt (value, minnode)) > + minnode = value; > + } > + } > + else > + minnode = maxnode = integer_zero_node; > + > + signop sgn = tree_int_cst_sgn (minnode) >= 0 ? UNSIGNED : SIGNED; > + int lowprec = tree_int_cst_min_precision (minnode, sgn); > + int highprec = tree_int_cst_min_precision (maxnode, sgn); > + prec = MAX (lowprec, highprec); > + return prec; > +} > + > /* FIELD is a bit-field. We are finishing the processing for its > enclosing type. Issue any appropriate messages and set appropriate > flags. Returns false if an error has been diagnosed. */ > @@ -3326,7 +3380,7 @@ check_bitfield_decl (tree field) > "width of %qD exceeds its type", field); > else if (TREE_CODE (type) == ENUMERAL_TYPE) > { > - int prec = TYPE_PRECISION (ENUM_UNDERLYING_TYPE (type)); > + int prec = enum_min_precision (type); > if (compare_tree_int (w, prec) < 0) > warning_at (DECL_SOURCE_LOCATION (field), 0, > "%qD is too small to hold all values of %q#T", > --- gcc/testsuite/g++.dg/cpp0x/enum23.C.jj 2013-02-16 09:31:06.400506406 +0100 > +++ gcc/testsuite/g++.dg/cpp0x/enum23.C 2019-11-25 16:51:19.532332223 +0100 > @@ -5,5 +5,5 @@ enum class MyEnum { A = 1 }; > > struct MyClass > { > - MyEnum Field1 : 3; // { dg-bogus "warning: 'MyClass::Field1' is too small" "" { xfail *-*-* } } > + MyEnum Field1 : 3; // { dg-bogus "warning: 'MyClass::Field1' is too small" } > }; > --- gcc/testsuite/g++.dg/cpp0x/enum38.C.jj 2019-11-25 16:50:56.323686060 +0100 > +++ gcc/testsuite/g++.dg/cpp0x/enum38.C 2019-11-25 16:52:28.466281278 +0100 > @@ -0,0 +1,25 @@ > +// PR c++/61414 > +// { dg-do compile { target c++11 } } > + > +enum C { C0 = -4, C1 = 3 }; > +enum D { D0 = 0, D1 = 15 }; > +enum class E { E0 = -4, E1 = 3 }; > +enum F : unsigned { F0 = 0, F1 = 15 }; > +enum __attribute__((__mode__ (__QI__))) G { G0 = -4, G1 = 3 }; > +enum __attribute__((__mode__ (__HI__))) H { H0 = 0, H1 = 15 }; > + > +struct S > +{ > + C a : 2; // { dg-warning "'S::a' is too small to hold all values of 'enum C'" } > + C b : 3; // { dg-bogus "'S::b' is too small to hold all values of 'enum C'" } > + D c : 3; // { dg-warning "'S::c' is too small to hold all values of 'enum D'" } > + D d : 4; // { dg-bogus "'S::d' is too small to hold all values of 'enum D'" } > + E e : 2; // { dg-warning "'S::e' is too small to hold all values of 'enum class E'" } > + E f : 3; // { dg-bogus "'S::f' is too small to hold all values of 'enum class E'" } > + F g : 3; // { dg-warning "'S::g' is too small to hold all values of 'enum F'" } > + F h : 4; // { dg-bogus "'S::h' is too small to hold all values of 'enum F'" } > + G i : 2; // { dg-warning "'S::i' is too small to hold all values of 'enum G'" } > + G j : 3; // { dg-bogus "'S::j' is too small to hold all values of 'enum G'" } > + H k : 3; // { dg-warning "'S::k' is too small to hold all values of 'enum H'" } > + H l : 4; // { dg-bogus "'S::l' is too small to hold all values of 'enum H'" } > +}; > > > Jakub > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. 2017-10-17 7:14 ` Martin Sebor 2017-10-18 13:54 ` Sam van Kampen via gcc-patches @ 2017-10-19 19:22 ` Eric Gallager 2017-10-19 23:07 ` Martin Sebor 1 sibling, 1 reply; 16+ messages in thread From: Eric Gallager @ 2017-10-19 19:22 UTC (permalink / raw) To: Martin Sebor; +Cc: Sam van Kampen, gcc-patches On 10/16/17, Martin Sebor <msebor@gmail.com> wrote: > On 10/16/2017 06:37 AM, Sam van Kampen via gcc-patches wrote: >> ..I just realised that the clang flag is -Wbitfield-enum-conversion, not >> -Wenum-bitfield-conversion. Please apply the patch below instead, which >> has replaced the two words to remove the inconsistency. >> >> 2017-10-16 Sam van Kampen <sam@segfault.party> >> >> * c-family/c.opt: Add a warning flag for struct bit-fields >> being too small to hold enumerated types. >> * cp/class.c: Likewise. >> * doc/invoke.texi: Add documentation for said warning flag. >> >> Index: gcc/c-family/c.opt >> =================================================================== >> --- gcc/c-family/c.opt (revision 253784) >> +++ gcc/c-family/c.opt (working copy) >> @@ -346,6 +346,10 @@ Wframe-address >> C ObjC C++ ObjC++ Var(warn_frame_address) Warning LangEnabledBy(C ObjC >> C++ ObjC++,Wall) >> Warn when __builtin_frame_address or __builtin_return_address is used >> unsafely. >> >> +Wbitfield-enum-conversion >> +C++ Var(warn_bitfield_enum_conversion) Init(1) Warning >> +Warn about struct bit-fields being too small to hold enumerated types. > > Warning by default is usually reserved for problems that are > most likely indicative of bugs. Does this rise to that level? > (It seems that it should be in the same class as -Wconversion). The warning is already enabled by default; this patch just adds the new flag controlling it. I don't think it's worth changing it away from warning by default when it already warns by default now. > >> + >> Wbuiltin-declaration-mismatch >> C ObjC C++ ObjC++ Var(warn_builtin_declaraion_mismatch) Init(1) Warning >> Warn when a built-in function is declared with the wrong signature. >> Index: gcc/cp/class.c >> =================================================================== >> --- gcc/cp/class.c (revision 253784) >> +++ gcc/cp/class.c (working copy) >> @@ -3278,10 +3278,11 @@ check_bitfield_decl (tree field) >> && tree_int_cst_lt (TYPE_SIZE (type), w))) >> warning_at (DECL_SOURCE_LOCATION (field), 0, >> "width of %qD exceeds its type", field); >> - else if (TREE_CODE (type) == ENUMERAL_TYPE >> + else if (warn_bitfield_enum_conversion >> + && TREE_CODE (type) == ENUMERAL_TYPE >> && (0 > (compare_tree_int >> (w, TYPE_PRECISION (ENUM_UNDERLYING_TYPE (type)))))) >> - warning_at (DECL_SOURCE_LOCATION (field), 0, >> + warning_at (DECL_SOURCE_LOCATION (field), >> OPT_Wbitfield_enum_conversion, >> "%qD is too small to hold all values of %q#T", >> field, type); > > I would suggest to include a bit more detail in the message, such > as the smallest number of bits needed to represent every value of > the enumeration. It's also often helpful to include a note > pointing to the relevant declaration (in this case it could be > the bit-field). > >> } >> Index: gcc/doc/invoke.texi >> =================================================================== >> --- gcc/doc/invoke.texi (revision 253784) >> +++ gcc/doc/invoke.texi (working copy) >> @@ -264,7 +264,7 @@ Objective-C and Objective-C++ Dialects}. >> -Walloca -Walloca-larger-than=@var{n} @gol >> -Wno-aggressive-loop-optimizations -Warray-bounds >> -Warray-bounds=@var{n} @gol >> -Wno-attributes -Wbool-compare -Wbool-operation @gol >> --Wno-builtin-declaration-mismatch @gol >> +-Wbitfield-enum-conversion -Wno-builtin-declaration-mismatch @gol >> -Wno-builtin-macro-redefined -Wc90-c99-compat -Wc99-c11-compat @gol >> -Wc++-compat -Wc++11-compat -Wc++14-compat @gol >> -Wcast-align -Wcast-align=strict -Wcast-qual @gol >> @@ -5431,6 +5431,12 @@ Incrementing a boolean is invalid in C++17, and de >> >> This warning is enabled by @option{-Wall}. >> >> +@item -Wbitfield-enum-conversion >> +@opindex Wbitfield-enum-conversion >> +@opindex Wno-bitfield-enum-conversion >> +Warn about a bit-field potentially being too small to hold all values >> +of an enumerated type. This warning is enabled by default. >> + > > The brief description makes me wonder what this really means. What > is the meaning of "potentially?" What (what expressions) triggers > the warning? Presumably it's initialization and assignment. Does > explicit or implicit conversion also trigger is? I would suggest > to expand the documentation to obviate these questions, and perhaps > also include an example. > > Martin > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414. 2017-10-19 19:22 ` [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414 Eric Gallager @ 2017-10-19 23:07 ` Martin Sebor 0 siblings, 0 replies; 16+ messages in thread From: Martin Sebor @ 2017-10-19 23:07 UTC (permalink / raw) To: Eric Gallager; +Cc: Sam van Kampen, gcc-patches On 10/19/2017 12:38 PM, Eric Gallager wrote: > On 10/16/17, Martin Sebor <msebor@gmail.com> wrote: >> On 10/16/2017 06:37 AM, Sam van Kampen via gcc-patches wrote: >>> ..I just realised that the clang flag is -Wbitfield-enum-conversion, not >>> -Wenum-bitfield-conversion. Please apply the patch below instead, which >>> has replaced the two words to remove the inconsistency. >>> >>> 2017-10-16 Sam van Kampen <sam@segfault.party> >>> >>> * c-family/c.opt: Add a warning flag for struct bit-fields >>> being too small to hold enumerated types. >>> * cp/class.c: Likewise. >>> * doc/invoke.texi: Add documentation for said warning flag. >>> >>> Index: gcc/c-family/c.opt >>> =================================================================== >>> --- gcc/c-family/c.opt (revision 253784) >>> +++ gcc/c-family/c.opt (working copy) >>> @@ -346,6 +346,10 @@ Wframe-address >>> C ObjC C++ ObjC++ Var(warn_frame_address) Warning LangEnabledBy(C ObjC >>> C++ ObjC++,Wall) >>> Warn when __builtin_frame_address or __builtin_return_address is used >>> unsafely. >>> >>> +Wbitfield-enum-conversion >>> +C++ Var(warn_bitfield_enum_conversion) Init(1) Warning >>> +Warn about struct bit-fields being too small to hold enumerated types. >> >> Warning by default is usually reserved for problems that are >> most likely indicative of bugs. Does this rise to that level? >> (It seems that it should be in the same class as -Wconversion). > > The warning is already enabled by default; this patch just adds the > new flag controlling it. I don't think it's worth changing it away > from warning by default when it already warns by default now. The C++ warning in its present form is considered unhelpful by users (as evident from the bug report). That it warns by default is part of the problem. GCC (in C mode), Clang, and Intel ICC warn only for enums whose enumerators cannot fit in the bitfield. (In Clang the warning is in addition disabled by default and has to be explicitly enabled.) The suggestion is to add a two-level warning and adjust G++ to warn the same way at level 1 as other compilers do (and to include this level in -Wall, or if it's considered appropriate and necessary, enable it by default), and warn as it does now only at level 2 (with that level having to be explicitly requested). I would further suggest to do the same in GCC (in C mode), if only for consistency. Martin ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-11-26 21:52 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-16 11:20 [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414 Sam van Kampen via gcc-patches 2017-10-16 13:26 ` Sam van Kampen via gcc-patches 2017-10-17 0:17 ` Joseph Myers 2017-10-18 13:59 ` Sam van Kampen via gcc-patches 2017-10-17 7:14 ` Martin Sebor 2017-10-18 13:54 ` Sam van Kampen via gcc-patches 2017-10-18 16:00 ` Martin Sebor 2017-10-18 19:29 ` Sam van Kampen via gcc-patches 2017-10-19 3:23 ` Martin Sebor 2017-10-20 16:05 ` Jason Merrill 2019-11-25 14:40 ` Jakub Jelinek 2019-11-25 14:54 ` Jakub Jelinek 2019-11-25 23:42 ` [C++ PATCH] Fix up is too small to hold all values of enum warning (PR c++/61414) Jakub Jelinek 2019-11-26 21:52 ` Jason Merrill 2017-10-19 19:22 ` [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414 Eric Gallager 2017-10-19 23:07 ` Martin Sebor
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).