[cfe-commits] [Windows] Use thiscall as the default calling convention for class methods

[Timur Iskhodzhanov]

Attached is an updated version.

It's not clear to me if I did the right thing near CGCall.cpp:136 and :160
I don't fully understand what was going on there but without these two
changes the explicitly-__cdecl function definition's ExtInfo had its
CC reset to CC_Default. This in turn forced my code at :353 to make
the wrong decision to use thiscall instead.
I'd be glad if you could check if I did stuff correctly there.

One more question - what do you think about this extra "bool isInstance" ?
Maybe this flag should be stored in ExtInfo and initialized in AST instead?

[ btw, I've never mentioned this review is about fixing
http://llvm.org/bugs/show_bug.cgi?id=12785 ]

--
Thanks,
Timur

On Tue, May 15, 2012 at 10:34 PM, John McCall <rjmccall at apple.com> wrote:
> On May 15, 2012, at 11:09 AM, Timur Iskhodzhanov wrote:
>> On Fri, May 11, 2012 at 9:55 PM, John McCall <rjmccall at apple.com> wrote:
>>> On May 11, 2012, at 6:58 AM, Timur Iskhodzhanov wrote:
>>>> Can you please review the attached patch?
>>>>
>>>> In two words what it does is
>>>> it overrides the CC_Default calling convention with
>>>> context.getDefaultMethodCallConv()
>>>> for all the used method definitions and calls.
>>>>
>>>> What I personally don't like is that the method declarations are
>>>> stored with CC_Default and we have to replace that with
>>>> context.getDefaultMethodCallConv() wherever we use it.
>>>> Unfortunately, we can't change the CC in the global declarations storage.
>>>> I'd prefer to put the declarations into the storage with the right CC
>>>> but not sure:
>>>> a) if we should do it
>>>> b) how to do it
>>>> c) it's possible at all
>>>
>>> I don't think modifying the AST is the right thing to do here anyway;
>>> CC_Default is really the right way to model this.
>> ok
>>
>>>> OTOH, it looks like there are only two places we need to alter the
>>>> calling convention: method call and method definition :)
>>>> So maybe I'm trying too hard to generalize...
>>> I mostly like your patch, but I think it'd be slightly better to make the
>>> terminal arrangeFunctionType take some sort of "function kind"
>>> parameter (function vs. C++ instance method) and then have
>>> ClangCallConvToLLVMCallConv consider that when mapping
>>> CC_Default.
>>>
>>> Also, please put this in a more generically-named test, something like
>>> microsoft-abi-methods.cpp.  I'm sure there will be other changes that
>>> we'll want to test for as time goes on.
>> I got your point.
>> I'll try doing that - I've started with tests and this has
>> uncovered a few bugs in ctor/dtor handling.
>> Will update as soon as it's fixed.
>
> Thanks!
>
>>> Also, please test:
>>>  - declarations of instance methods using CC_Default
>> you mean, declarations without explicitly specified CC ?
>
> Yes.
>
>>>  - declarations of instance methods with an explicit CC
>> Would you prefer __thiscall/__cdecl or __attribute__ ?
>
> If we predefine __thiscall in MS mode, then testing that way is fine.
>
> John.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_12785_3.patch
Type: application/octet-stream
Size: 10673 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120517/35f22387/attachment.obj>