From: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>
To: Martin Sebor <msebor@gmail.com>
Cc: gcc Patches <gcc-patches@gcc.gnu.org>,
Marek Polacek <polacek@redhat.com>,
"Joseph S. Myers" <joseph@codesourcery.com>
Subject: Re: [1/2] PR 78736: New warning -Wenum-conversion
Date: Tue, 09 May 2017 13:25:00 -0000 [thread overview]
Message-ID: <CAAgBjMnithBB0WKerdbRFp8zs01bzXS7an8UvPQcjAYBPMo57g@mail.gmail.com> (raw)
In-Reply-To: <CAAgBjMnwWMW378HZyHorO6ohbBQ5c2SGiyOeQ+qHixSL=2aOFg@mail.gmail.com>
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
next prev parent reply other threads:[~2017-05-09 13:24 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-02 17:13 Prathamesh Kulkarni
2017-05-02 22:10 ` Martin Sebor
2017-05-03 6:10 ` Prathamesh Kulkarni
2017-05-09 13:25 ` Prathamesh Kulkarni [this message]
2017-05-09 18:44 ` Martin Sebor
2017-05-09 21:19 ` Pedro Alves
2017-05-10 13:15 ` Prathamesh Kulkarni
2017-05-10 15:18 ` Martin Sebor
2017-06-12 20:17 ` Joseph Myers
2017-07-11 12:29 ` Prathamesh Kulkarni
2017-07-12 15:33 ` Sandra Loosemore
2017-07-31 18:40 ` Prathamesh Kulkarni
2017-08-08 4:21 ` Prathamesh Kulkarni
2017-08-17 12:53 ` Prathamesh Kulkarni
2017-08-26 19:27 ` Joseph Myers
2017-09-01 2:37 ` Prathamesh Kulkarni
2017-09-01 11:55 ` Joseph Myers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAAgBjMnithBB0WKerdbRFp8zs01bzXS7an8UvPQcjAYBPMo57g@mail.gmail.com \
--to=prathamesh.kulkarni@linaro.org \
--cc=gcc-patches@gcc.gnu.org \
--cc=joseph@codesourcery.com \
--cc=msebor@gmail.com \
--cc=polacek@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).