[PATCH] Bug 14984 - Implement __attribute__((ms_abi))

[Reid Kleckner]

Yep, LGTM.


On Thu, Aug 29, 2013 at 4:04 PM, Charles Davis <cdavis5x at gmail.com> wrote:

>
> On Aug 29, 2013, at 4:28 PM, Reid Kleckner wrote:
>
> On Thu, Aug 29, 2013 at 3:09 PM, Charles Davis <cdavis5x at gmail.com> wrote:
>
>>
>> On Aug 29, 2013, at 11:32 AM, Reid Kleckner wrote:
>>
>> I think the lesson of CC_Default vs. CC_C is that there are *far* too
>> many places to check for calling convention compatibility, and that it's
>> much better to use a single representation for equivalent conventions.
>>
>> Therefore, I'd remove areCCsCompatible() and let
>> TargetInfo::checkCallingConvention() adjust the the sysv or MS CCs to CC_C
>> when appropriate.
>>
>> This way, when you target x64 Windows, the ms_abi attr will show up in
>> the AST, but it will produce function types that are nicely canonically
>> equivalent to normal function types.  Similarly, sysv_abi will be a no-op
>> on sysv targets.
>>
>> You mean, like this?
>>
>> There's one side effect of your suggested change that still bugs me,
>> though. Now, if we're on a target that uses the SysV calling convention,
>> the diagnostics always say "cdecl", even if the function was explicitly
>> declared "sysv_abi". (Same with Windows and "ms_abi".) This might be
>> slightly confusing to your average user, but for obvious reasons I don't
>> want to add special cases to the places these diagnostics get printed.
>>
>
> I think the right way to handle this is to build AttributedType nodes for
> the no-op attribute, but let equivalent and modified types be the same.
>  The type printer should use __attribute__((sysv_abi/ms_abi)).  Looking at
> SemaType, it's not clear if this happens.
>
> Can you write a test for the bad case with a FIXME maybe?
>
> There already is one (Clang :: Sema/ms_abi-sysv_abi.c). I'll add a FIXME
> there.
>
> Otherwise, OK to commit?
>
> Chip
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130829/fcdab623/attachment.html>