Re: [1/2] PR 78736: New warning -Wenum-conversion

[Martin Sebor <msebor at gmail dot com>]

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, ...

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.

Martin