<div dir="ltr">Thanks for the reminder, Rafael's patch from Friday fell out of my inbox.<div><br></div><div>This looks good to me.  So long as we reject the bogus code, getting the diagnostic right on these corner cases is a QOI thing.</div>
<div><br></div><div><div>-        if (Context.hasSameType(Function->getType(), Method->getType())) {</div><div>+        QualType Adjusted = Function->getType();</div>
<div>+        const AttributedType *AT = Adjusted->getAs<AttributedType>();</div><div><br></div><div>I think the right thing to do here is something like the dyn_cast<> + IgnoreParens loop in adjustMemberFunctionCC.  We're really trying to ask the question: "was there an explicit CC attribute on *this* decl, ignoring attributes on typedefs or template type args?".  That could be it's own helper.</div>
<div><br></div><div>AttributedType is a non-canonical type, so we'll only be able to find it if it is the outermost type.  getAs<> only looks at the most sugared type and the canonical type.  It doesn't single step desugar in a loop.</div>
<div><br></div><div>I tried to invent a test, but MSVC doesn't like parens on the inside of its calling convention attributes and crashes when you get typedefs involved.  =/</div><div><br></div><div>+        if (!AT || !AT->isCallingConv())</div>
<div>+          Adjusted = adjustCCAndNoReturn(Adjusted, Method->getType());</div>
<div>+        if (Context.hasSameType(Adjusted, Method->getType())) {</div></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Dec 9, 2013 at 3:52 PM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, Dec 6, 2013 at 7:17 PM, Rafael Espíndola<br>
<<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
> On 5 December 2013 17:23, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:<br>
>> What happens for an invalid test case like:<br>
>><br>
>> template <typename T><br>
>> struct S {<br>
>>   void f(T t) {<br>
>>     t = 42;<br>
>>   }<br>
>> };<br>
>> template<> void __cdecl S<void*>::f(void*);<br>
>> void g(S<void*> s, void* p) {<br>
>>   s.f(p);<br>
>> }<br>
>><br>
>> MSVC rejects this with "error C2373: 'S<T>::f' : redefinition; different<br>
>> type modifiers".  Do we just paper that over and adjust to __thiscall?<br>
><br>
> With the previous patch we would still produce an error:<br>
><br>
> /Users/respindola/llvm/test.cpp:7:35: error: function declared 'cdecl'<br>
> here was previously declared 'thiscall'<br>
> template<> void __cdecl S<void*>::f(void*);<br>
>                                   ^<br>
> /Users/respindola/llvm/test.cpp:7:35: note: previous declaration is here<br>
><br>
> but clang was clearly getting confused as show by the note on the same<br>
> line as the error.<br>
><br>
>> Maybe the right thing is to use a comparison that knows how to overlook<br>
>> implicit CC differences, but knows about explicit CC differences.<br>
><br>
> The attached patch does that. With it clang prints an error about the<br>
> __cdecl, ignores the broken specialization and continues.<br>
<br>
</div></div>I don't know this code, so can't really contribute to the review here,<br>
but this patch makes the following tests pass in ms abi mode, which I<br>
really like :)<br>
<br>
CXX/temp/temp.spec/temp.expl.spec/p2-0x.cpp<br>
CXX/temp/temp.spec/temp.expl.spec/p2.cpp<br>
CXX/temp/temp.spec/temp.expl.spec/p21.cpp<br>
CXX/temp/temp.spec/temp.expl.spec/p4.cpp<br>
CXX/temp/temp.spec/temp.expl.spec/p5.cpp<br>
CXX/temp/temp.spec/temp.expl.spec/p6.cpp<br>
CXX/temp/temp.spec/temp.explicit/p4.cpp<br>
<span class="HOEnZb"><font color="#888888"><br>
 - Hans<br>
</font></span></blockquote></div><br></div>