Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414.

[Sam van Kampen via 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?

Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414.

[Sam van Kampen via gcc-patches]

On Mon, Oct 16, 2017 at 11:31:10PM +, 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
> jos...@codesourcery.com

Thanks for the feedback,
Sam

Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414.

[Sam van Kampen via 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 ty

Re: [patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414.

[Sam van Kampen via 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  

* 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

[patch, c++] Add a warning flag for the enum bit-field declaration warning in bug #61414.

[Sam van Kampen via 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  

* 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