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

[Andrew V. Tischenko via cfe-commits]

avt77 marked an inline comment as done.

================
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;
----------------
rsmith wrote:
> 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?
Yes, I see "dllimport gets turned into dllexport" for definitions only. 
No, the definition should not be inline: 
   if (OldImportAttr && !HasNewAttr && **!IsInline**

================
Comment at: lib/Sema/SemaDecl.cpp:5595
@@ +5594,3 @@
+      // Replace DLLImportAttr with DLLExportAttr
+      OldDecl->dropAttr<DLLImportAttr>();
+      NewDecl->dropAttr<DLLImportAttr>();
----------------
rsmith wrote:
> 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.
 I don't understand how I could "make sure that anyone...". Please, clarify. 

================
Comment at: lib/Sema/SemaDecl.cpp:5596
@@ +5595,3 @@
+      OldDecl->dropAttr<DLLImportAttr>();
+      NewDecl->dropAttr<DLLImportAttr>();
+      OldDecl->addAttr(::new (S.Context) DLLExportAttr(
----------------
rsmith wrote:
> Is this really valid and treated as `__dllexport` if the new declaration explicitly specifies `__dllimport` (rather than inheriting it)?
The new declaration does not have explicitly specified __dllimport attribute: 
if (OldImportAttr && **!HasNewAttr**. It's inherited.

================
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 {
----------------
rsmith wrote:
> The new attribute should be marked implicit.
What do you mean? Please, clarify.


http://reviews.llvm.org/D18953