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
