[cfe-commits] [Windows] Use thiscall as the default calling convention for class methods
timurrrr at google.com
Thu May 17 07:49:14 PDT 2012
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
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
>>>> 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.
>>>> 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
>>> 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.
>>> Also, please test:
>>> - declarations of instance methods using CC_Default
>> you mean, declarations without explicitly specified CC ?
>>> - 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 10673 bytes
Desc: not available
More information about the cfe-commits