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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to