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

David Majnemer david.majnemer at gmail.com
Fri Feb 7 16:41:40 PST 2014



================
Comment at: lib/Parse/ParsePragma.cpp:820
@@ +819,3 @@
+  }
+  PP.Lex(Tok);
+  const IdentifierInfo *Arg = Tok.getIdentifierInfo();
----------------
Richard Smith wrote:
> David Majnemer wrote:
> > 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)
> Crumbs. What about this:
> 
>     #define X full_generality, single_inheritance
>     #pragma pointers_to_members(X)
> 
> ... and ...
> 
>     #define X (best_case)
>     #pragma pointers_to_members X
> 
> ?
MSVC thinks this is perfectly cromulent.

================
Comment at: test/SemaCXX/member-pointer-ms.cpp:214
@@ +213,3 @@
+
+// Even if a definition proceedes the first mention of a pointer to member, we
+// still give the record the fully general representation.
----------------
Richard Smith wrote:
> "precedes"
Will do.

================
Comment at: lib/Parse/Parser.cpp:112
@@ -111,1 +111,3 @@
   }
+  if (getLangOpts().MSVCCompat) {
+    MSPointersToMembers.reset(new PragmaMSPointersToMembers());
----------------
Richard Smith wrote:
> David Majnemer wrote:
> > 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.
> Well, it's automatically conforming since it's a non-standard pragma. Choosing between `MSVCCompat` and `MicrosoftExt`, I think the only reason we'd have to pick the former is that we think this is a heinous extension (which, to be fair, it is, but we support the inheritance attribute keywords in `MicrosoftExt`).
> 
> I think the question is, does it make sense to have this pragma (and the keywords, for that matter) when building for the MS ABI with MS extensions disabled? Is that even a mode we care about?
> 
> (I'm happy with doing the most-restrictive thing for now and putting this under `MSVCCompat`, but I'm not sure that's the right long-term approach, and I'd like for us to have a more principled decision-making process here.)
My opinion has swayed, I think it makes sense for it to be `MicrosoftExt`.

================
Comment at: lib/Parse/ParsePragma.cpp:857-858
@@ +856,4 @@
+
+  if (!HaveRepresentationMethod ||
+      RepresentationMethod != Sema::PPTMK_BestCase) {
+    if (Arg->isStr("single_inheritance")) {
----------------
Richard Smith wrote:
> David Majnemer wrote:
> > 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.
> Ah, the 'full_generality' is optional if there's an inheritance model. I think this'd be clearer if this code is part of the `else` for 'best_case'. Also, a comment describing the grammar for this pragma would help a lot.
I'll try to move it.


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



More information about the cfe-commits mailing list