[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!

In https://reviews.llvm.org/D51340#1246508, @hans wrote:

> The static local stuff still makes me nervous. How much does Chromium break if we just ignore the problem? And how does -fvisibility-inlines-hidden handle the issue?


I'm not sure how much chromium will break, if we ignore static local variables. But ignoring static local var may cause some unhappy behavior and experience to developer.
I'd like to have check for local static variables as -fvisibility-inlines-hidden does.

-fvisibility-inlines-hidden checks static local around here.
https://github.com/llvm-project/llvm-project-20170507/blob/77b7ca42b0ea496373c5e5a9710b1fe11e1fba68/clang/lib/AST/Decl.cpp#L1265

> Also, Sema already has a check for static locals in C inline functions:
> 
>   $ echo "inline void f() { static int x; }" | bin/clang -c -x c -
>   <stdin>:1:19: warning: non-constant static local variable in inline function may be different in different files [-Wstatic-local-in-inline]
>   inline void f() { static int x; }
>                     ^
>   
> 
> 
> could we reuse that check somehow for this case with static locals in dllexport inline methods?

I think we can do the same thing when we are given -fno-dllexport-inlines around here.
https://github.com/llvm-project/llvm-project-20170507/blob/77b7ca42b0ea496373c5e5a9710b1fe11e1fba68/clang/lib/Sema/SemaDecl.cpp#L6661

But I bit prefer to do export functions with static local variables. Because that is consistent with -fvisibility-inlines-hidden and we won't need more changes to chromium than the CLs in description.



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5702
+            !getLangOpts().DllExportInlines &&
+            Class->getTemplateSpecializationKind() != TSK_ExplicitInstantiationDeclaration &&
+            Class->getTemplateSpecializationKind() != TSK_ExplicitInstantiationDefinition) {
----------------
hans wrote:
> What worries me is that now we have logic in two different places about inheriting the dll attribute.
> 
> The new place in ActOnFinishInlineFunctionDef doesn't have the same checks (checking for MS ABI and the template specialization kind) which means the logic for when to inherit is already a little out of sync...
Agree. Do you allow me to extract check to function and re-use that?


https://reviews.llvm.org/D51340