[PATCH] Bugs in SemaOverload.cpp

[Richard Smith]

I like Alp's suggestion of a .def file: the duplication and distribution of
this information has been a (low level) annoyance for quite some time and
it'd be good to properly fix it.


On Wed, Jul 2, 2014 at 1:09 AM, Dmitry Babokin <babokin at gmail.com> wrote:

> 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/20140708/e0808636/attachment.html>