[PATCH] [ms-cxxabi] Fix a number of bugs in the mangler.

[Peter Collingbourne]

================
Comment at: lib/AST/MicrosoftMangle.cpp:62
@@ -61,1 +61,3 @@
 public:
+  enum MangleQualifiers { MQ_Drop, MQ_Escape, MQ_Mangle, MQ_Result };
+
----------------
Timur Iskhodzhanov wrote:
> How about QualifiersManglingMode ?
> _ResultType / _ReturnType ?
> How about QualifiersManglingMode ?

OK, I gave this a better name.

> _ResultType / _ReturnType ?

I don't think there's a great name for this last variant, as it's also used for RTTI.  I would suggest we avoid bikeshedding over the name until we've more fully implemented the ABI.

================
Comment at: lib/AST/MicrosoftMangle.cpp:1014
@@ +1013,3 @@
+  if (MQ == MQ_Result) {
+    if ((Quals && !IsPointer) || T->getAs<TagType>()) {
+      Out << '?';
----------------
Timur Iskhodzhanov wrote:
> You've probably wanted `isa<TagType>(T)`
Yep.

================
Comment at: lib/AST/MicrosoftMangle.cpp:1029
@@ +1028,3 @@
+    mangleQualifiers(Quals, false);
+  } else if (!IsPointer && MQ == MQ_Escape && Quals) {
+    Out << "$$C";
----------------
Timur Iskhodzhanov wrote:
> Hm, the value of this condition does not change during a function.
> 
> I find it a bit better to localize an error-prone part of the function (i.e. overriding MQ in the middle of a function) by moving the `else if` part up, e.g.:
> 
>   if (const ArrayType ...) {  // OK, easy
>     ...
>   } else if (!IsPointer && MQ == MQ_Escape && Quals) {  // OK, easy
>     ...
>   }
>   
>   // Error-prone part begins, read carefully.
>   if (MQ == MQ_Result) {
>     ...
>       MQ = MQ_Mangle;
>     ...
>   }
>   if (MQ == MQ_Mangle) {
>     ...
>   }
>   // Error-prone part ends
> 
>   // OK, easy part continues
> 
> WDYT?
After I moved the MQ_Escape block, I realised that we can make this logic much easier to read using a switch statement.  Which I did :)

================
Comment at: test/CodeGenCXX/mangle-ms-arg-qualifiers.cpp:64
@@ +63,3 @@
+void foo_aad(char &x) {}
+// CHECK: "\01?foo_aad@@YAXAAD at Z"
+
----------------
Timur Iskhodzhanov wrote:
> (Not checking every char here, assuming you've compared these manglings with what CL expects)
Yep.

================
Comment at: test/CodeGenCXX/mangle-ms-arg-qualifiers.cpp:105
@@ +104,3 @@
+
+void foo_qay04cbh(const int x[5][5]) {}
+// CHECK: "\01?foo_qay04cbh@@YAXQAY04$$CBH at Z"
----------------
Reid Kleckner wrote:
> MQ_Result is used to mangle the return value, right?  Can we do just one test for a qualified return value?
I thought the coverage of this in mangle-ms-return-qualifiers.cpp was good, but I took another look and realised we weren't covering qualified pointers.  I also added a couple of cases (member pointers) that we weren't handling correctly before.

================
Comment at: lib/AST/MicrosoftMangle.cpp:1756
@@ +1755,3 @@
+                                           raw_ostream &Out) {
+  MicrosoftCXXNameMangler Mangler(*this, Out);
+  Mangler.getStream() << "\01??_R0";
----------------
Timur Iskhodzhanov wrote:
> Why is this a part of this patch?
Reverted.  I'm also working on a patch for basic RTTI, and I'll include these hunks in that one.


http://llvm-reviews.chandlerc.com/D709