On 10/16/17, Martin Sebor <mse...@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 >