[patch] Fix pr18141

[Reid Kleckner]

Thanks for the reminder, Rafael's patch from Friday fell out of my inbox.

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.

-        if (Context.hasSameType(Function->getType(), Method->getType())) {
+        QualType Adjusted = Function->getType();
+        const AttributedType *AT = Adjusted->getAs<AttributedType>();

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.

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.

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.
 =/

+        if (!AT || !AT->isCallingConv())
+          Adjusted = adjustCCAndNoReturn(Adjusted, Method->getType());
+        if (Context.hasSameType(Adjusted, Method->getType())) {



On Mon, Dec 9, 2013 at 3:52 PM, Hans Wennborg <hans at chromium.org> wrote:

> On Fri, Dec 6, 2013 at 7:17 PM, Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
> > On 5 December 2013 17:23, Reid Kleckner <rnk at google.com> wrote:
> >> What happens for an invalid test case like:
> >>
> >> template <typename T>
> >> struct S {
> >>   void f(T t) {
> >>     t = 42;
> >>   }
> >> };
> >> template<> void __cdecl S<void*>::f(void*);
> >> void g(S<void*> s, void* p) {
> >>   s.f(p);
> >> }
> >>
> >> MSVC rejects this with "error C2373: 'S<T>::f' : redefinition; different
> >> type modifiers".  Do we just paper that over and adjust to __thiscall?
> >
> > With the previous patch we would still produce an error:
> >
> > /Users/respindola/llvm/test.cpp:7:35: error: function declared 'cdecl'
> > here was previously declared 'thiscall'
> > template<> void __cdecl S<void*>::f(void*);
> >                                   ^
> > /Users/respindola/llvm/test.cpp:7:35: note: previous declaration is here
> >
> > but clang was clearly getting confused as show by the note on the same
> > line as the error.
> >
> >> Maybe the right thing is to use a comparison that knows how to overlook
> >> implicit CC differences, but knows about explicit CC differences.
> >
> > The attached patch does that. With it clang prints an error about the
> > __cdecl, ignores the broken specialization and continues.
>
> I don't know this code, so can't really contribute to the review here,
> but this patch makes the following tests pass in ms abi mode, which I
> really like :)
>
> CXX/temp/temp.spec/temp.expl.spec/p2-0x.cpp
> CXX/temp/temp.spec/temp.expl.spec/p2.cpp
> CXX/temp/temp.spec/temp.expl.spec/p21.cpp
> CXX/temp/temp.spec/temp.expl.spec/p4.cpp
> CXX/temp/temp.spec/temp.expl.spec/p5.cpp
> CXX/temp/temp.spec/temp.expl.spec/p6.cpp
> CXX/temp/temp.spec/temp.explicit/p4.cpp
>
>  - Hans
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131209/1f84d6b8/attachment.html>