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

[Eric Gallager <egall at gwmail dot gwu dot edu>]

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
>