On 26 August 2017 at 04:15, Joseph Myers <joseph@codesourcery.com> wrote:
> On Tue, 11 Jul 2017, Prathamesh Kulkarni wrote:
>
>> On 13 June 2017 at 01:47, Joseph Myers <joseph@codesourcery.com> wrote:
>> > 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.)
>> Hi,
>> Sorry for the late response, I was on a vacation.
>> The attached patch is rebased and bootstrap+tested on x86_64-unknown-linux-gnu.
>> I have modified it slightly to not warn for enums with different names
>> but having same value ranges.
>> For eg:
>> enum e1 { e1_1, e1_2 };
>> enum e2 { e2_1, e2_2 };
>>
>> enum e1 x = e2_1;
>> With this version, there would be no warning for the above assignment
>> since both e1 and e2 have
>> same value ranges.  Is that OK ?
>
> I don't really think that's a good heuristic.  I see no particular reason
> to think that just because two enums have the same set of values, an
> implicit conversion between them is actually deliberate - corresponding
> values have the same semantics in some sense - rather than reflecting an
> underlying confusion in the code.  (You could have different levels of the
> warning - and only warn in this case at a higher level - but I don't
> really think that makes sense either for this particular warning.)
Thanks for the suggestion, I have reverted this change in the attached patch.
>
>> The patch has following fallouts in the testsuite:
>>
>> a) libgomp:
>> I initially assume it was a false positive because I thought enum
>> gomp_schedule_type
>> and enum omp_sched_t have same value-ranges but it looks like omp_sched_t
>> has range [1, 4] while gomp_schedule_type has range [0, 4] with one
>> extra element.
>> Is the warning then correct for this case ?
>>
>> b) libgfortran:
>> i) Implicit conversion from unit_mode to file_mode
>> ii) Implicit conversion from unit_sign_s to unit_sign.
>> I suppose the warning is OK for these cases since unit_mode, file_mode
>> have different value-ranges and similarly for unit_sign_s, unit_sign ?
>
> If it's an implicit conversion between different enum types, the warning
> is correct.  The more important question for the review is: is it correct
> to replace the implicit conversion by an explicit one?  That is, for each
> value in the source type, what enumerator of the destination type has the
> same value, and do the semantics match in whatever way is required for the
> code in question?
Well, for instance unit_sign in libgfortran/io.h is defined as:
typedef enum
{ SIGN_PROCDEFINED, SIGN_SUPPRESS, SIGN_PLUS, SIGN_UNSPECIFIED }
unit_sign;

and unit_sign_s is defined:
typedef enum
{ SIGN_S, SIGN_SS, SIGN_SP }
unit_sign_s;

Since the enum types are different, I assume warnings for implicit
conversion from unit_sign_s to unit_sign would be correct ?
And since unit_sign_s is a subset of unit_sign in terms of
value-ranges, I assume replacing implicit by explicit conversion would
be OK ?

Thanks,
Prathamesh
>
> Also note s/valeu/value/ in the documentation.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com