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

[Takuto Ikuta via Phabricator via cfe-commits]

takuto.ikuta added a comment.

Thank you for review! I updated the code.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:11976
+
+    while (FD && !getDLLAttr(FD) &&
+           !FD->hasAttr<DLLExportStaticLocalAttr>() &&
----------------
hans wrote:
> Why does this need to be a loop? I don't think FunctionDecl's can be nested?
This is for static local var in lambda function.
static_x's ParentFunction does not become f().
```
class __declspec(dllexport) C {
  int f() {
    return ([]() { static int static_x; return ++static_x; })();
  }
};
```


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11995
+
+        // Function having static local variables should be exported.
+        auto *ExportAttr = cast<InheritableAttr>(NewAttr->clone(getASTContext()));
----------------
hans wrote:
> Isn't it enough that the static local is exported, does the function itself need to be exported too?
Adding dllexport only to variable does not export variable when the function is not used.
But dllimport is not necessary for function, removed.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5705
+            TSK != TSK_ExplicitInstantiationDeclaration &&
+            TSK != TSK_ExplicitInstantiationDefinition) {
+          if (ClassExported) {
----------------
hans wrote:
> 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?
> But I don't understand why the template stuff is checked here...

Templated inline function is not always inlined, it seems depending on optimization level.

I updated the code as you wrote in later part of comment.


https://reviews.llvm.org/D51340