On 3 May 2017 at 03:28, Martin Sebor <msebor@gmail.com> wrote:
On 05/02/2017 11:11 AM, Prathamesh Kulkarni wrote:
Hi,
The attached patch attempts to add option -Wenum-conversion for C and
objective-C similar to clang, which warns when an enum value of a type
is implicitly converted to enum value of another type and is enabled
by Wall.
It seems quite useful. My only high-level concern is with
the growing number of specialized warnings and options for each
and their interaction.
I've been working on -Wenum-assign patch that complains about
assigning to an enum variables an integer constants that doesn't
match any of the enumerators of the type. Testing revealed that
the -Wenum-assign duplicated a subset of warnings already issued
by -Wconversion enabled with -Wpedantic. I'm debating whether
to suppress that part of -Wenum-assign altogether or only when
-Wconversion and -Wpedantic are enabled.
My point is that these dependencies tend to be hard to discover
and understand, and the interactions tricky to get right (e.g.,
avoid duplicate warnings for similar but distinct problems).
This is not meant to be a negative comment on your patch, but
rather a comment about a general problem that might be worth
starting to think about.
One comment on the patch itself:
+ warning_at_rich_loc (&loc, 0, "implicit conversion from"
+ " enum type of %qT to %qT", checktype, type);
Unlike C++, the C front end formats an enumerated type E using
%qT as 'enum E' so the warning prints 'enum type of 'enum E'),
duplicating the "enum" part.
I would suggest to simplify that to:
warning_at_rich_loc (&loc, 0, "implicit conversion from "
"%qT to %qT", checktype, ...
Thanks for the suggestions. I have updated the patch accordingly.
Hmm the issue you pointed out of warnings interaction is indeed of concern.
I was wondering then if we should merge this warning with -Wconversion
instead of having a separate option -Wenum-conversion ? Although that will not
really help with your example below.
Martin
PS As an example to illustrate my concern above, consider this:
enum __attribute__ ((packed)) E { e1 = 1 };
enum F { f256 = 256 };
enum E e = f256;
It triggers -Woverflow:
warning: large integer implicitly truncated to unsigned type [-Woverflow]
enum E e = f256;
^~~~
also my -Wenum-assign:
warning: integer constant ‘256’ converted to ‘0’ due to limited range [0,
255] of type ‘‘enum E’’ [-Wassign-enum]
enum E e = f256;
^~~~
and (IIUC) will trigger your new -Wenum-conversion.
Yep, on my branch it triggered -Woverflow and -Wenum-conversion.
Running the example on clang shows a single warning, which they call
as -Wconstant-conversion, which
I suppose is similar to your -Wassign-enum.