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

Message ID CAAgBjM=S2EjhqVize+nBjgb_aCd3m+TqdOEp7XBC6hfcm-6tDw@mail.gmail.com
State New
Headers show
Series
  • [1/2] PR 78736: New warning -Wenum-conversion
Related show

Commit Message

Prathamesh Kulkarni May 2, 2017, 5:11 p.m.
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.

Bootstrapped+tested on x86_64-unknown-linux-gnu.
Is the patch OK for trunk ?

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

Comments

Martin Sebor May 2, 2017, 9:58 p.m. | #1
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
Prathamesh Kulkarni May 3, 2017, 6 a.m. | #2
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
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." } */
+}

Prathamesh Kulkarni May 9, 2017, 1:24 p.m. | #3
ping https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00161.html

Thanks,
Prathamesh

On 3 May 2017 at 11:30, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
> 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
Martin Sebor May 9, 2017, 6:04 p.m. | #4
On 05/09/2017 07:24 AM, Prathamesh Kulkarni wrote:
> ping https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00161.html

>

> Thanks,

> Prathamesh

>

> On 3 May 2017 at 11:30, Prathamesh Kulkarni

> <prathamesh.kulkarni@linaro.org> wrote:

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


-Wassign-enum is a Clang warning too, it just isn't included in
either -Wall or -Wextra.  It warns when a constant is assigned
to a variable of an enumerated type and is not representable in
it.  I enhanced it for GCC to also warn when the constant doesn't
correspond to an enumerator in the type, but I'm starting to think
that rather than adding yet another option to GCC it might be better
to extend your -Wenum-conversion once it's committed to cover those
cases (and also to avoid issuing multiple warnings for essentially
the same problem).  Let me ponder that some more.

I can't approve patches but it looks good to me for the most part.
There is one minor issue that needs to be corrected:

+	  gcc_rich_location loc (location);
+	  warning_at_rich_loc (&loc, 0, "implicit conversion from"
+			       " %qT to %qT", checktype, type);

Here the zero should be replaced with OPT_Wenum_conversion,
otherwise the warning option won't be included in the message.

Martin

>>

>> 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
Pedro Alves May 9, 2017, 9:13 p.m. | #5
On 05/09/2017 07:04 PM, Martin Sebor wrote:
>>>

> 

> -Wassign-enum is a Clang warning too, it just isn't included in

> either -Wall or -Wextra.  It warns when a constant is assigned

> to a variable of an enumerated type and is not representable in

> it.  I enhanced it for GCC to also warn when the constant doesn't

> correspond to an enumerator in the type,


Note that that means that the warning will trigger in the common use
case of using enums as bit flags, like:

enum flags
  {
    F1 = 1 << 1,
    F2 = 1 << 2,
    F3 = 1 << 3
  };

void foo ()
{
  enum flags f = 0;
  f = F1 | F2;
}

I was going to suggest adding an attribute to mark such enum
types, and it turns out that Clang has one already:
  https://clang.llvm.org/docs/AttributeReference.html#flag-enum

So, IMHO if we add the warning, IWBN to add the attribute as well.

> but I'm starting to think

> that rather than adding yet another option to GCC it might be better

> to extend your -Wenum-conversion once it's committed to cover those

> cases (and also to avoid issuing multiple warnings for essentially

> the same problem).  Let me ponder that some more.


Thanks,
Pedro Alves
Prathamesh Kulkarni May 10, 2017, 12:19 p.m. | #6
On 9 May 2017 at 23:34, Martin Sebor <msebor@gmail.com> wrote:
> On 05/09/2017 07:24 AM, Prathamesh Kulkarni wrote:

>>

>> ping https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00161.html

>>

>> Thanks,

>> Prathamesh

>>

>> On 3 May 2017 at 11:30, Prathamesh Kulkarni

>> <prathamesh.kulkarni@linaro.org> wrote:

>>>

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

>

>

> -Wassign-enum is a Clang warning too, it just isn't included in

> either -Wall or -Wextra.  It warns when a constant is assigned

> to a variable of an enumerated type and is not representable in

> it.  I enhanced it for GCC to also warn when the constant doesn't

> correspond to an enumerator in the type, but I'm starting to think

> that rather than adding yet another option to GCC it might be better

> to extend your -Wenum-conversion once it's committed to cover those

> cases (and also to avoid issuing multiple warnings for essentially

> the same problem).  Let me ponder that some more.

>

> I can't approve patches but it looks good to me for the most part.

> There is one minor issue that needs to be corrected:

>

> +         gcc_rich_location loc (location);

> +         warning_at_rich_loc (&loc, 0, "implicit conversion from"

> +                              " %qT to %qT", checktype, type);

>

> Here the zero should be replaced with OPT_Wenum_conversion,

> otherwise the warning option won't be included in the message.

Oops, sorry about that, updated in the attached patch.
In the patch, I have left the warning in Wall, however I was wondering
whether it should be
in Wextra instead ?
The warning triggered for icv.c in libgomp for following assignment:
icv->run_sched_var = kind;

because icv->run_sched_var was of type enum gomp_schedule_type and
'kind' was of type enum omp_sched_t.
However although these enums have different names, they are
structurally identical (same values),
so the warning in this case, although not a false positive, seems a
bit artificial ?

Thanks,
Prathamesh
>

> Martin

>

>

>>>

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

>

>
2017-05-10  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..d7570835c61 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, OPT_Wenum_conversion, "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." } */
+}

Martin Sebor May 10, 2017, 2:53 p.m. | #7
On 05/10/2017 06:19 AM, Prathamesh Kulkarni wrote:
> On 9 May 2017 at 23:34, Martin Sebor <msebor@gmail.com> wrote:

>> On 05/09/2017 07:24 AM, Prathamesh Kulkarni wrote:

>>>

>>> ping https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00161.html

>>>

>>> Thanks,

>>> Prathamesh

>>>

>>> On 3 May 2017 at 11:30, Prathamesh Kulkarni

>>> <prathamesh.kulkarni@linaro.org> wrote:

>>>>

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

>>

>>

>> -Wassign-enum is a Clang warning too, it just isn't included in

>> either -Wall or -Wextra.  It warns when a constant is assigned

>> to a variable of an enumerated type and is not representable in

>> it.  I enhanced it for GCC to also warn when the constant doesn't

>> correspond to an enumerator in the type, but I'm starting to think

>> that rather than adding yet another option to GCC it might be better

>> to extend your -Wenum-conversion once it's committed to cover those

>> cases (and also to avoid issuing multiple warnings for essentially

>> the same problem).  Let me ponder that some more.

>>

>> I can't approve patches but it looks good to me for the most part.

>> There is one minor issue that needs to be corrected:

>>

>> +         gcc_rich_location loc (location);

>> +         warning_at_rich_loc (&loc, 0, "implicit conversion from"

>> +                              " %qT to %qT", checktype, type);

>>

>> Here the zero should be replaced with OPT_Wenum_conversion,

>> otherwise the warning option won't be included in the message.

> Oops, sorry about that, updated in the attached patch.

> In the patch, I have left the warning in Wall, however I was wondering

> whether it should be

> in Wextra instead ?

> The warning triggered for icv.c in libgomp for following assignment:

> icv->run_sched_var = kind;

>

> because icv->run_sched_var was of type enum gomp_schedule_type and

> 'kind' was of type enum omp_sched_t.

> However although these enums have different names, they are

> structurally identical (same values),

> so the warning in this case, although not a false positive, seems a

> bit artificial ?


I'd say the warning is justified in this case, even if the two
enums are clearly designed to be interchangeable.  It will be
a reminder to review code like it to make sure it is, in fact
intended and correct.  If it is, it's easy to suppress by
an explicit cast.  So based on this example alone I wouldn't
feel compelled to remove it from -Wall just yet.

FWIW, when I'm concerned about the impact of a new warning I
build a few packages with it (usually Binutils, Glibc, the
Linux kernel, GDB, Bash, and Busybox).  That gives me some
sense of what to expect.  In the end it's a judgment call as
different people have varying degrees of tolerance for these
kinds of warnings.

Martin

>

> Thanks,

> Prathamesh

>>

>> Martin

>>

>>

>>>>

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

>>

>>
Joseph Myers June 12, 2017, 8:17 p.m. | #8
This is OK with one fix:

> +C ObjC Var(warn_enum_conversion) Init(0) Warning LangEnabledBy(C Objc,Wall)


I believe the LangEnabledBy arguments are case-sensitive, so you need to 
have ObjC not Objc there for it to work correctly.  (*.opt parsing isn't 
very good at detecting typos and giving errors rather than silently 
ignoring things.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Patch hide | download patch | download mbox

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..c9cde8d7fef 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"
+			       " enum type of %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..4459109c7cb
--- /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 type of .enum Y. to .enum X." } */
+enum Y obj2 = y1;
+
+enum X obj3;
+void foo()
+{
+  obj3 = y2; /* { dg-warning "implicit conversion from enum type of .enum Y. to .enum X." } */
+}
+
+void bar(enum X);
+void f(void)
+{
+  bar (y1); /* { dg-warning "implicit conversion from enum type of .enum Y. to .enum X." } */
+}