[PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: lib/Sema/SemaDecl.cpp:5570
@@ -5565,3 +5569,3 @@
   // and qualified friend declarations.
-  // NB: MSVC converts such a declaration to dllexport.
+  // NB: MSVC converts such a declaration to dllexport that's why we do it also.
   bool IsInline = false, IsStaticDataMember = false, IsQualifiedFriend = false;
----------------
Just describe this as the semantics of this situation rather than suggesting this is some MSVC oddity we're emulating. "In such a case, the declaration is treated as if it were marked dllexport." or similar.

It also seems bizarre for this behavior to apply for local extern declarations and qualified friend declarations. Does the "dllimport gets turned into dllexport" behavior actually only apply to the definition case? And does the definition need to be inline?

================
Comment at: lib/Sema/SemaDecl.cpp:5595
@@ +5594,3 @@
+      // Replace DLLImportAttr with DLLExportAttr
+      OldDecl->dropAttr<DLLImportAttr>();
+      NewDecl->dropAttr<DLLImportAttr>();
----------------
Don't change attributes on a prior declaration; AST nodes should generally be immutable once created (this would lose source fidelity, and break under PCH/modules). Instead, make sure that anyone who looks at this gets the attribute from the appropriate (most recent) declaration and only change the attributes there.

================
Comment at: lib/Sema/SemaDecl.cpp:5596
@@ +5595,3 @@
+      OldDecl->dropAttr<DLLImportAttr>();
+      NewDecl->dropAttr<DLLImportAttr>();
+      OldDecl->addAttr(::new (S.Context) DLLExportAttr(
----------------
Is this really valid and treated as `__dllexport` if the new declaration explicitly specifies `__dllimport` (rather than inheriting it)?

================
Comment at: lib/Sema/SemaDecl.cpp:5600-5602
@@ +5599,5 @@
+          OldImportAttr->getSpellingListIndex()));
+      NewDecl->addAttr(::new (S.Context) DLLExportAttr(
+          NewImportAttr->getRange(), S.Context,
+          NewImportAttr->getSpellingListIndex()));
+    } else {
----------------
The new attribute should be marked implicit.


http://reviews.llvm.org/D18953