[PATCH] D42768: AST: add an extension to support SwiftCC on MS ABI

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.


================
Comment at: lib/AST/MicrosoftMangle.cpp:1061
+    if (Ty->getAs<FunctionType>()->getCallConv() == CC_Swift)
+      for (const char *NS : {"__swift_cc", "__Swift"})
+        mangleSourceName(NS);
----------------
rsmith wrote:
> Do we really need both of these qualifiers? This seems redundant to me.
I'd rather keep both.  The `__Swift` is to preserve this entire area for the swift extensions.  The `__swift_cc` is the specific feature.  In the future, if we need something else, we could do `__Swift::__new_feature`.  Alternatively, we could introduce a large number of namespaces, but, I think trying to use as little as possible is nice.


================
Comment at: lib/AST/MicrosoftMangle.cpp:1717
+        Out << '8';
+        for (const char *NS : {"__swift_cc", "__Swift"})
+          mangleSourceName(NS);
----------------
erichkeane wrote:
> rsmith wrote:
> > erichkeane wrote:
> > > When I implemented regcall, it was brought up that this was likely a bad idea, and to just choose a letter for mangleCallingConvention.  It is a really stable list as it is, and if you avoid ones in the immediate pattern, you'll be fine.
> > If you can get someone at Microsoft to sign off on us using a specific letter, that seems fine. Otherwise, because we don't define the ABI, we don't get to invent extensions to it, and we should instead pick something (like this) that we can be confident won't conflict with future official mangling extensions
> Hmm... well, when we did it with RegCall, we didn't get them to 'sign off' so to speak, but simply alerted them about it after the fact and they promised to keep an eye on it.
This is a fairly well established technique within clang, we already use this for a large number of C types (e.g. `_Complex`, `_Atomic`, etc) which MSVC does not yet support and may at some point.


Repository:
  rC Clang

https://reviews.llvm.org/D42768