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

[Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 3215 bytes --]

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.

test-eg.c:3:12: warning: implicit conversion from 'int' to 'enum E'
changes value from 256 to 0 [-Wconstant-conversion]
enum E e = f256;
       ~   ^~~~

Thanks,
Prathamesh
>
> Martin

[-- Attachment #2: pr78736-4-gcc.txt --]
[-- Type: text/plain, Size: 3872 bytes --]

2017-05-02  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

	* doc/invoke.text: Document Wenum-conversion.
	* c-family/c.opt (Wenum-conversion): New option.
	* c/c-typeck.c (convert_for_assignment): Handle Wenum-conversion.

testsuite/
	* gcc.dg/Wenum-conversion.c: New test-case.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 9ad2f6e1fcc..e04312ec253 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -492,6 +492,10 @@ Wenum-compare
 C ObjC C++ ObjC++ Var(warn_enum_compare) Init(-1) Warning LangEnabledBy(C ObjC,Wall || Wc++-compat)
 Warn about comparison of different enum types.
 
+Wenum-conversion
+C ObjC Var(warn_enum_conversion) Init(0) Warning LangEnabledBy(C Objc,Wall)
+Warn about implicit conversion of enum types.
+
 Werror
 C ObjC C++ ObjC++
 ; Documented in common.opt
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 6f9909c6396..483b2008f7b 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -6309,6 +6309,20 @@ convert_for_assignment (location_t location, location_t expr_loc, tree type,
 	}
     }
 
+  if (warn_enum_conversion)
+    {
+      tree checktype = origtype != NULL_TREE ? origtype : rhstype;
+      if (checktype != error_mark_node
+	  && TREE_CODE (checktype) == ENUMERAL_TYPE
+	  && TREE_CODE (type) == ENUMERAL_TYPE
+	  && TYPE_MAIN_VARIANT (checktype) != TYPE_MAIN_VARIANT (type))
+	{
+	  gcc_rich_location loc (location);
+	  warning_at_rich_loc (&loc, 0, "implicit conversion from"
+			       " %qT to %qT", checktype, type);
+	}
+    }
+
   if (TYPE_MAIN_VARIANT (type) == TYPE_MAIN_VARIANT (rhstype))
     return rhs;
 
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 0eeea7b3b87..79b1e175374 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -273,7 +273,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wdisabled-optimization @gol
 -Wno-discarded-qualifiers  -Wno-discarded-array-qualifiers @gol
 -Wno-div-by-zero  -Wdouble-promotion  -Wduplicated-cond @gol
--Wempty-body  -Wenum-compare  -Wno-endif-labels  -Wexpansion-to-defined @gol
+-Wempty-body  -Wenum-compare -Wenum-conversion  -Wno-endif-labels  -Wexpansion-to-defined @gol
 -Werror  -Werror=*  -Wextra-semi  -Wfatal-errors @gol
 -Wfloat-equal  -Wformat  -Wformat=2 @gol
 -Wno-format-contains-nul  -Wno-format-extra-args  @gol
@@ -3754,6 +3754,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}.
 -Wcomment  @gol
 -Wduplicate-decl-specifier @r{(C and Objective-C only)} @gol
 -Wenum-compare @r{(in C/ObjC; this is on by default in C++)} @gol
+-Wenum-conversion @r{in C/ObjC;} @gol
 -Wformat   @gol
 -Wint-in-bool-context  @gol
 -Wimplicit @r{(C and Objective-C only)} @gol
@@ -5961,6 +5962,12 @@ In C++ enumerated type mismatches in conditional expressions are also
 diagnosed and the warning is enabled by default.  In C this warning is 
 enabled by @option{-Wall}.
 
+@item -Wenum-conversion @r{(C, Objective-C only)}
+@opindex Wenum-conversion
+@opindex Wno-enum-conversion
+Warn when an enum value of a type is implicitly converted to an enum of
+another type. This warning is enabled by @option{-Wall}.
+
 @item -Wextra-semi @r{(C++, Objective-C++ only)}
 @opindex Wextra-semi
 @opindex Wno-extra-semi
diff --git a/gcc/testsuite/gcc.dg/Wenum-conversion.c b/gcc/testsuite/gcc.dg/Wenum-conversion.c
new file mode 100644
index 00000000000..86033399b7d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wenum-conversion.c
@@ -0,0 +1,20 @@
+/* { dg-do compile } */
+/* { dg-options "-Wenum-conversion" } */
+
+enum X { x1, x2 };
+enum Y { y1, y2 };
+
+enum X obj = y1;  /* { dg-warning "implicit conversion from .enum Y. to .enum X." } */
+enum Y obj2 = y1;
+
+enum X obj3;
+void foo()
+{
+  obj3 = y2; /* { dg-warning "implicit conversion from .enum Y. to .enum X." } */
+}
+
+void bar(enum X);
+void f(void)
+{
+  bar (y1); /* { dg-warning "implicit conversion from .enum Y. to .enum X." } */
+}