[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Thanks! I think this is getting close now.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:11976
+
+    while (FD && !getDLLAttr(FD) &&
+           !FD->hasAttr<DLLExportStaticLocalAttr>() &&
----------------
Why does this need to be a loop? I don't think FunctionDecl's can be nested?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11995
+
+        // Function having static local variables should be exported.
+        auto *ExportAttr = cast<InheritableAttr>(NewAttr->clone(getASTContext()));
----------------
Isn't it enough that the static local is exported, does the function itself need to be exported too?


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5703
+        if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+            !getLangOpts().DllExportInlines &&
+            TSK != TSK_ExplicitInstantiationDeclaration &&
----------------
I don't think checking for Microsoft ABI is necessary, just checking the DllExportInlines flag should be enough.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5705
+            TSK != TSK_ExplicitInstantiationDeclaration &&
+            TSK != TSK_ExplicitInstantiationDefinition) {
+          if (ClassExported) {
----------------
But I don't understand why the template stuff is checked here...

The way I imagined this, we'd only need to change the code when creating NewAttr below..
Something like

```
NewAttr = ClassAtr->clone()...
if (!getLandOpts().DllExportInlines() && Member is an inline method)
  NewAttr = our new dllimport/export static locals attribute
```

What do you think?


https://reviews.llvm.org/D51340