PR16752[PATCH]: 'mode' attribute for unusual targets doesn't work properly.

[Eli Friedman]

LGTM.

-Eli


On Mon, Sep 9, 2013 at 5:04 AM, Stepan Dyatkovskiy <stpworld at narod.ru>wrote:

> Since I had split the patch and committed only ASTContext and TargetInfo
> getIntTypeByWidth and friends, there is the second patch, that fixes
> PR16752 itself.
>
> Please, find it in attachment for review.
>
> -Stepan.
> Eli Friedman wrote:
>
>> On Tue, Sep 3, 2013 at 2:16 AM, Stepan Dyatkovskiy <stpworld at narod.ru
>> <mailto:stpworld at narod.ru>> wrote:
>>
>>     Hi Eli,
>>     Sorry for latency.
>>     As you remember this patch should correct 'mode' attr
>>     implementation. You proposed to use generic way of type detection
>>     for each target: just scan for target types and select one with
>>     suitable width.
>>
>>     Unfortunately I can't commit patch with generic implementation. I
>>     got test failure for clang. And I expect more failures for another
>>     targets. The reason is next.
>>
>>     The target could still keep mode unsupported even if it has type of
>>     suitable width. I mean the next.
>>     For example this string should cause error for i686 machines (128
>>     bit float):
>>     typedef          float f128ibm __attribute__ ((mode (TF)));
>>     But in case of generic approach for all targets it would be passed.
>>     In this case virtual functions allows to implement exceptions.
>>
>>
>> The generic implementation is still essentially correct; the the issue
>> is that getLongDoubleWidth() isn't actually the right value to check.
>>   It returns "sizeof(long double) * 8", not the actual width of the
>> underlying float format.  You should be able to work around this by
>> checking getLongDoubleFormat() instead of getLongDoubleWidth().
>>
>>     In this case 'getIntTypeByWidth' may be a bit confusing name.
>>     Instead it is supposed to return type for some particular mode, not
>>     by width. Something like getInt/RealTypeForMode(width, sign).
>>     Though, currently I kept the original name.
>>
>>
>> If you want to change it, that's fine.
>>
>> -Eli
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130909/5861d837/attachment.html>