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

[Prathamesh Kulkarni <prathamesh dot kulkarni at linaro dot org>]

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

Attachment: pr78736-5-gcc.txt
Description: Text document