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

[Timur Iskhodzhanov]

  Great patch!
  Just a few minor suggestions.

  Also please give me some time to try this patch on some real code - I'll update on this thread soon.


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

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

================
Comment at: lib/AST/MicrosoftMangle.cpp:1022
@@ -1002,5 +1021,3 @@
 
-  // If we're mangling a qualified array type, push the qualifiers to
-  // the element type.
-  if (split.Quals && isa<ArrayType>(T)) {
-    ty = Context.getASTContext().getAsArrayType(T);
+  if (MQ == MQ_Mangle) {
+    if (const FunctionType *FT = T->getAs<FunctionType>()) {
----------------
It would be nice to add a comment reminding the reader of the code that MQ has just been changed, so we need to re-read it (so, switch/else-if are not appropriate).
A simple comment would suffice, its main goal is to give a "pay attention here" message.

================
Comment at: lib/AST/MicrosoftMangle.cpp:1029
@@ +1028,3 @@
+    mangleQualifiers(Quals, false);
+  } else if (!IsPointer && MQ == MQ_Escape && Quals) {
+    Out << "$$C";
----------------
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?

================
Comment at: lib/AST/MicrosoftMangle.cpp:1756
@@ +1755,3 @@
+                                           raw_ostream &Out) {
+  MicrosoftCXXNameMangler Mangler(*this, Out);
+  Mangler.getStream() << "\01??_R0";
----------------
Why is this a part of this patch?

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


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