On 9 May 2017 at 23:34, Martin Sebor 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 >> wrote: >>> >>> On 3 May 2017 at 03:28, Martin Sebor 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 > >