[PATCH] MS ABI: Add support for #pragma pointers_to_members

[David Majnemer]

================
Comment at: lib/Parse/ParsePragma.cpp:820
@@ +819,3 @@
+  }
+  PP.Lex(Tok);
+  const IdentifierInfo *Arg = Tok.getIdentifierInfo();
----------------
Richard Smith wrote:
> Have you checked whether macro expansion should be performed on this token?
Yes, the following is supposed to work:

  #define X best_case
  #pragma pointers_to_members(X)

================
Comment at: lib/Parse/ParsePragma.cpp:837-840
@@ +836,6 @@
+    RepresentationMethod = Sema::PPTMK_FullGeneralityVirtualInheritance;
+  }
+
+  if (HaveRepresentationMethod &&
+      RepresentationMethod != Sema::PPTMK_BestCase) {
+    if (Tok.is(tok::comma)) {
----------------
Richard Smith wrote:
> Delete these four lines? =)
Will do.

================
Comment at: lib/Parse/ParsePragma.cpp:857-858
@@ +856,4 @@
+
+  if (!HaveRepresentationMethod ||
+      RepresentationMethod != Sema::PPTMK_BestCase) {
+    if (Arg->isStr("single_inheritance")) {
----------------
Richard Smith wrote:
> Maybe fold this into the `if` above too?
This `if` handles the cases where we don't have a representation method or the representation method is full generality.

The above `if` only handles the full generality case.

================
Comment at: lib/Parse/ParsePragma.cpp:888-890
@@ +887,5 @@
+
+  Token *Toks = (Token *)PP.getPreprocessorAllocator().Allocate(
+      sizeof(Token) * 1, llvm::alignOf<Token>());
+  new (Toks) Token();
+  Toks[0].startToken();
----------------
Richard Smith wrote:
> `Token *Tok = new (PP.getPreprocessorAllocator()) Token;`
Will do.

================
Comment at: lib/Parse/ParsePragma.cpp:896-897
@@ +895,4 @@
+      reinterpret_cast<void *>(static_cast<uintptr_t>(RepresentationMethod)));
+  PP.EnterTokenStream(Toks, 1, /*DisableMacroExpansion=*/true,
+                      /*OwnsTokens=*/false);
+}
----------------
Richard Smith wrote:
> Can you use `EnterToken` instead of `EnterTokenStream` here?
Will do.

================
Comment at: lib/Parse/Parser.cpp:112
@@ -111,1 +111,3 @@
   }
+  if (getLangOpts().MSVCCompat) {
+    MSPointersToMembers.reset(new PragmaMSPointersToMembers());
----------------
Richard Smith wrote:
> Why `MSVCCompat` rather than `MicrosoftExt`, or something based on whether we're using the MS ABI?
I felt that it is a non-conforming extension. I can key it off of the ABI if you think that is more appropriate.

================
Comment at: lib/Sema/SemaType.cpp:5109
@@ +5108,3 @@
+                    PPTMK_BestCase,
+                RD->getSourceRange()));
+          }
----------------
Richard Smith wrote:
> If the inheritance model came from a 'full generality' pragma, we should probably use the location of the pragma as the location of the implicit attribute.
Will do.

================
Comment at: test/Misc/warning-flags.c:19
@@ -18,3 +18,3 @@
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
----------------
Richard Smith wrote:
> Please add a `-W` flag covering your warning! Maybe add one `-W` flag for all these warn_pragma_blah warnings? `-Wignored-pragma`, to match `-Wignored-attribute`?
Will do.

================
Comment at: test/SemaCXX/member-pointer-ms.cpp:206-209
@@ +205,6 @@
+
+#pragma pointers_to_members(full_generality, multiple_inheritance)
+struct TrulySingleInheritance;
+static_assert(sizeof(int TrulySingleInheritance::*) == kMultipleDataSize, "");
+#pragma pointers_to_members(best_case)
+// This definition shouldn't conflict with the increased generality that the
----------------
Richard Smith wrote:
> Seems you can only have one `#pragma pointers_to_members` per TU in MSVC land. You seem to have some complexity to deal with the possibility of the pragma changing (for instance, storing the `BestCase` flag on the attribute) that may be unnecessary?
It is necessary because of MSVC's vmg/vms/vmm/vmv flags.  I don't understand why they have their restriction.

================
Comment at: lib/Parse/ParsePragma.cpp:865
@@ +864,3 @@
+      RepresentationMethod = Sema::PPTMK_FullGeneralityVirtualInheritance;
+    } else if (Tok.isNot(tok::r_paren)) {
+      PP.Diag(Tok.getLocation(),
----------------
Richard Smith wrote:
> `Tok` can't be `tok::r_paren` here, because we already know it's got `IdentifierInfo`.
This is needed to handle the case of `#pragma pointers_to_members(virtual_inheritance)`

================
Comment at: include/clang/Sema/Sema.h:273
@@ +272,3 @@
+  /// \brief Controls member pointer representation format under the MS ABI.
+  PragmaMSPointersToMembersKind MSPointerToMemberRepresentationMethod;
+
----------------
Richard Smith wrote:
> You seem to be missing serialization/deserialization code for this.
> 
> How are we going to handle this w/modules?
Good point, will look into this.

================
Comment at: include/clang/Sema/Sema.h:6984
@@ +6983,3 @@
+  /// ActOnPragmaMSPointersToMembers - called on well formed \#pragma
+  /// pointers_to_members(representation method, [general purose
+  /// representation]).
----------------
Reid Kleckner wrote:
> s/purose/purpose/
Will fix.

================
Comment at: lib/Sema/Sema.cpp:214-215
@@ -212,3 +213,4 @@
   if (VisContext) FreeVisContext();
   MSStructPragmaOn = false;
+  MSPointerToMemberRepresentationMethod = PPTMK_BestCase;
   // Kill all the active scopes.
----------------
Richard Smith wrote:
> Why are we messing with either of these in the `Sema` destructor?
Fixed.


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