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 <[email protected]> wrote: > On May 15, 2012, at 11:09 AM, Timur Iskhodzhanov wrote: >> On Fri, May 11, 2012 at 9:55 PM, John McCall <[email protected]> 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.
bug_12785_3.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
