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

[Timur Iskhodzhanov]

================
Comment at: lib/AST/MicrosoftMangle.cpp:62
@@ -61,1 +61,3 @@
 public:
+  enum MangleQualifiers { MQ_Drop, MQ_Escape, MQ_Mangle, MQ_Result };
+
----------------
Peter Collingbourne wrote:
> 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.
> I would suggest we avoid bikeshedding over the name until we've more fully implemented the ABI.

Agreed!

================
Comment at: lib/AST/MicrosoftMangle.cpp:1029
@@ +1028,3 @@
+    mangleQualifiers(Quals, false);
+  } else if (!IsPointer && MQ == MQ_Escape && Quals) {
+    Out << "$$C";
----------------
Peter Collingbourne wrote:
> 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 :)
Great, the logic is so much easier to follow now!

================
Comment at: lib/AST/MicrosoftMangle.cpp:1015
@@ -1002,5 +1014,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);
+  switch (QMM) {
+  case QMM_Drop:
----------------
please add

  default:
    llvm_unreachable("Unknown QMM type");

or something like that.


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

BRANCH
  ms-mangler-bugs

ARCANIST PROJECT
  clang