[PATCH] MS ABI: Implement #pragma vtordisp() and clang-cl /vdN

[Reid Kleckner]

================
Comment at: lib/Parse/ParsePragma.cpp:192
@@ -191,1 +191,3 @@
 
+void Parser::HandlePragmaMSVtorDisp() {
+  assert(Tok.is(tok::annot_pragma_ms_vtordisp));
----------------
David Majnemer wrote:
> This seems a bit unusual compared to the other `#pragma` implementations.  Looking at the very similar `Parser::HandlePragmaPack`, it consumes a single token which contains a pointer to useful data. A similar approach should make the implementation of `Parser::HandlePragmaMSVtorDisp` simpler.
HandlePragmaPack is somewhat simple, but the other side has to use the PP allocator and placement new to push the magic token, which is gross.  Why not just push the integer or identifier token in question and let the parser consume tokens like it normally would?  Also, pragma pack doesn't do any error handling if the integer value is bogus, which is half of the complexity here.

Long term, I think the pragma handlers in parse should receive a reference to Sema or Parser so they don't need to inject tokens.  Then we can handle things like:

  struct A {
    virtual void f(); 
  #pragma vtordisp(0)
     struct B : virtual A { virtual void f(); };
  };

Today we error our because the annotations don't appear at the right places in our grammar.

================
Comment at: include/clang/Driver/CLCompatOptions.td:121
@@ -120,1 +120,3 @@
+def _SLASH_vd : CLJoined<"vd">, HelpText<"Control vtordisp placement">,
+  Alias<vtordisp_mode_EQ>;
 def _SLASH_Z7 : CLFlag<"Z7">, Alias<gline_tables_only>;
----------------
David Majnemer wrote:
> Can we have a test in ##test/Driver/cl-options.c## for checking that `-vd` works?
> 
> Also, my cl doesn't allow stuff like `cl -vd1 -vd2 q.cpp`. It errors with:
> > '/vd1' and '/vd2' command-line options are incompatible
> 
> 
We already have a test to make sure we accept /vdN without error.  I don't think it's worth a separate process invocation in every test suite run just to make sure that tablegen aliases work.

================
Comment at: lib/Sema/SemaAttr.cpp:133
@@ -132,4 +132,3 @@
 void Sema::AddMsStructLayoutForRecord(RecordDecl *RD) {
-  if (!MSStructPragmaOn)
-    return;
-  RD->addAttr(MsStructAttr::CreateImplicit(Context));
+  if (MSStructPragmaOn) {
+    RD->addAttr(MsStructAttr::CreateImplicit(Context));
----------------
David Majnemer wrote:
> This `if` and the one that follows don't need braces around their statements.
Sure.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2729
@@ +2728,3 @@
+  // vftables.
+  if (RD->getMSVtorDispMode() == 2) {
+    for (CXXRecordDecl::base_class_const_iterator I = RD->vbases_begin(),
----------------
David Majnemer wrote:
> Would it be better to use a symbolic constant instead of magic numbers?
Sure.


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