[PATCH] Start adding support for dllimport/dllexport on classes (PR11170)

Richard Smith richard at metafoo.co.uk
Tue May 27 16:51:26 PDT 2014


Looks basically fine. Just a few comments.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:1395
@@ +1394,3 @@
+      // Don't dllexport/import destructor thunks.
+      F->setDLLStorageClass(llvm::GlobalValue::DefaultStorageClass);
+    }
----------------
This would make more sense to me in `SetFunctionAttributes`, where (presumably) the DLL storage class is set.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:4394
@@ +4393,3 @@
+  // template specialization bases.
+  for (Decl *D : Class->decls()) {
+    if (VarDecl *VD = dyn_cast<VarDecl>(D)) {
----------------
What about member classes? Are they implicitly re-exported too?

================
Comment at: lib/Sema/SemaDeclCXX.cpp:4404
@@ +4403,3 @@
+
+      if (ClassExported && !MD->isDefaulted() && !MD->isDeleted()) {
+        // Instantiate non-default methods.
----------------
`ClassExported && MD->isUserProvided()` maybe

================
Comment at: lib/Sema/SemaDeclCXX.cpp:4407-4408
@@ +4406,4 @@
+        S.MarkFunctionReferenced(Class->getLocation(), MD);
+      } else if (ClassExported && !MD->isDeleted() &&
+                 (!MD->isTrivial() || MD->isCopyAssignmentOperator())) {
+        // Also instantiate non-trivial default methods and the copy assignment
----------------
Weird! You get an exported trivial copy assignment but not an exported trivial copy constructor? I guess we'll need to update this once we find out what they do with implicit move operations?

================
Comment at: lib/Sema/SemaDeclCXX.cpp:4415
@@ +4414,3 @@
+        S.ResolveExceptionSpec(Class->getLocation(), FPT);
+        S.ActOnFinishInlineMethodDef(MD);
+      }
----------------
Why do we need this here but not in the non-defaulted case?

http://reviews.llvm.org/D3877






More information about the cfe-commits mailing list