[PATCH] Bugs in SemaOverload.cpp

[Dmitry Babokin]

Alp,

To me both approaches look good, as they are eliminating possibility of
future errors. I don't have preference for one or another. I think it's up
to file maintainers (Richard?) to decide what approach to follow.

By the way, my patch also contains fixes for missing cases in these
routines. Are these fixes matches with what you have in your changes? Do
they look correct to you?

Dmitry.


On Tue, Jul 1, 2014 at 11:33 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 01/07/2014 22:00, Alp Toker wrote:
>
>> Hi Dmitry,
>>
>> I agree the current setup isn't ideal.
>>
>> An alternative to your approach is to convert this into an X macro table
>> (or just a big rectangular array perhaps). Something like:
>>
>> ImplicitConversions.def:
>>
>>   IC(Kind, Name, Category, Rank):
>>   IC(Identity, "No conversion", Identity, Exact_Match)
>>   IC(Lvalue_To_Rvalue, "Lvalue-to-rvalue", Lvalue_Transformation,
>> Exact_Match)
>>   ...
>>
>>
> In other words, that'd be similar to what we already do for overloaded
> operators:
>
>   include/clang/Basic/OperatorKinds.def
>
> Alp.
>
>
>
>> (We've made this change in our clang derivative, which implements several
>> new conversion kinds, and the above format is serving us well because it
>> also acts as documentation for the language extension, making it easy to
>> see the ranks at a glance.)
>>
>> What do you think?
>>
>> Alp.
>>
>>
>> On 01/07/2014 14:56, Dmitry Babokin wrote:
>>
>>> Hi rsmith,
>>>
>>> I've noticed that GetConversionCategory(), GetConversionRank() and
>>> GetImplicitConversionName() from lib/Sema/SemaOverload.cpp have a bunch of
>>> bugs, namely:
>>>   - GetConversionCategory() is missing cases for ICK_TransparentUnionConversion,
>>> ICK_Writeback_Conversion and ICK_Zero_Event_Conversion cases (ok to not
>>> handle ICK_Num_Conversion_Kinds, though would be good to report an error);
>>>   - GetConversionRank() is missing the case for
>>> ICK_Zero_Event_Conversion;
>>>   - GetImplicitConversionName() doesn't have a comma after "Transparent
>>> Union Conversion";
>>>
>>> I think it's better to rewrite these functions using switch, so adding
>>> new ICKs would trigger a warning or an error if it's not handled in these
>>> functions.
>>>
>>> Except style changes and typo fix (missing comma), the changes add case
>>> for missing records, please verify that they've got correct values.
>>>
>>> I have no commit rights, so I would appreciate if reviewer commits the
>>> patch. Thanks!
>>>
>>> http://reviews.llvm.org/D4354
>>>
>>> Files:
>>>    lib/Sema/SemaOverload.cpp
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140702/5810e636/attachment.html>