[PATCH] Allow dllimport/dllexport on inline functions and get the linkage right

[Hans Wennborg]

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2095
@@ -2094,1 +2094,3 @@
+def err_attribute_dllimport_function_definition : Error<
+  "dllimport cannot by applied to non-inline function definition">;
 def err_attribute_dllimport_data_definition : Error<
----------------
Yunzhong Gao wrote:
> Typo. "cannot by applied" => "cannot be applied"
Thanks for catching! Fixed.

================
Comment at: lib/AST/ASTContext.cpp:7802
@@ +7801,3 @@
+static GVALinkage fixGVALinkageForDLLAttribute(GVALinkage L, const Decl *D) {
+  if (D->hasAttr<DLLImportAttr>()) {
+    if (L == GVA_DiscardableODR || L == GVA_StrongODR)
----------------
Yunzhong Gao wrote:
> Reid Kleckner wrote:
> > This should have a comment about the semantics of dllimport and dllexport, and link to the MSDN doc on dllexport with inline functions:
> > http://msdn.microsoft.com/en-us/library/xa0d9ste.aspx
> If the first argument to fixGVALinkageForDLLAttribute() is always the return result of
> basicGVALinkageForFunction(), I wonder maybe it is more readable to just call the
> basic linkage function from the fix linkage function.
> 
> Do we still check somewhere that we do not accept the combination of C99/GNU-inline with dllexport/dllimport?
No, with this patch we allow dllimport/export on all inline functions.

Reid pointed out that we would get the linkage wrong for exported functions in C99 mode, and that the reasonable thing to do would be to use MS semantics dllimport/export inline functions. I'll update the patch to do this.

================
Comment at: lib/Sema/SemaDecl.cpp:9774
@@ +9773,3 @@
+
+    if (!DA->isInherited() && !FD->isInlined()) {
+      Diag(FD->getLocation(), diag::err_attribute_dllimport_function_definition);
----------------
Yunzhong Gao wrote:
> Do you have a test case for the isInherited() case where the dllimport definition should be allowed?
The original code had it, so I figured it was important for some reason.

The check goes all the way back to r61437, and it's not entirely clear what is was for even then. Back then it was possible to have both dllimport and dllexport attributes on the same function, and I suspect the isInherited check has to do with that.

I don't think that check is correct today, so I'm going to remove it and simplify the code here a bit.

http://reviews.llvm.org/D3772