[PATCH] D12402: PR24595: clang-cl fails to compile vswriter.h header from Windows SDK 8.1 in 32 bit mode

[Reid Kleckner via cfe-commits]

rnk added inline comments.

================
Comment at: lib/Sema/SemaType.cpp:5855-5856
@@ -5854,3 +5854,4 @@
   CallingConv CurCC = FT->getCallConv();
-  CallingConv FromCC =
+  CallingConv DefaultCC =
       Context.getDefaultCallingConvention(IsVariadic, IsStatic);
+  CallingConv ToCC;
----------------
DefaultCC is only meaningful when we're dealing with non-structor member functions. In the structor case, you're conflating it with ToCC. Let's sink the DefaultCC definition down into the 'else' block, since it's only meaningful there.

================
Comment at: lib/Sema/SemaType.cpp:5857
@@ -5856,5 +5856,3 @@
       Context.getDefaultCallingConvention(IsVariadic, IsStatic);
-  CallingConv ToCC = Context.getDefaultCallingConvention(IsVariadic, !IsStatic);
-  if (CurCC != FromCC || FromCC == ToCC)
-    return;
+  CallingConv ToCC;
 
----------------
ToCC is always the same in both cases:
  CallingConv ToCC = Context.getDefaultCallingConvention(IsVariadic, !IsStatic);
Let's just keep that here, and return early if CurrCC == ToCC, since the common case is that we don't need to do anything.

================
Comment at: lib/Sema/SemaType.cpp:5864-5865
@@ +5863,4 @@
+    // bit MS ABI environment. This probably should be fixed.
+    if (Context.getTargetInfo().getTriple().isArch32Bit())
+      DefaultCC = CC_X86ThisCall;
+
----------------
This is reimplementing ASTContext::getDefaultCallingConv. Let's not duplicate that logic.

================
Comment at: lib/Sema/SemaType.cpp:5867-5868
@@ +5866,4 @@
+
+    if (CurCC == DefaultCC)
+      return;
+
----------------
This can be covered by returning earlier if CurrCC == ToCC.

================
Comment at: lib/Sema/SemaType.cpp:5870-5872
@@ +5869,5 @@
+
+    // __declspec(dllexport) got transformed to CC_C and allowed.
+    if (CurCC == CC_C)
+      return;
+
----------------
This seems wrong, see this test case:
  struct A {
    __cdecl A(int x);
    int f;
  };
  A::A(int x) : f(x) {}
32-bit MSVC turns it into thiscall.

I think what you're really seeing is that variadic constructors need to stay cdecl. getDefaultCallingConvention already handles this case.

================
Comment at: lib/Sema/SemaType.cpp:5873-5877
@@ +5872,7 @@
+      return;
+
+    ToCC = DefaultCC;
+
+    // Issue a warning on ignored calling convention -- except of __stdcall.
+    // Again, this is what MS compiler does.
+    if (CurCC != CC_X86StdCall)
----------------
andreybokhanko wrote:
> I tried, but this leads to massive stability fails. I'm willing to investigate and fix them (if needed), but in a separate patch, OK?
I'd really rather get the *structor calling convention logic right on the first try.


http://reviews.llvm.org/D12402