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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

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?

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?



================
Comment at: clang/lib/Sema/SemaDecl.cpp:12624
+      isa<CXXMethodDecl>(D)) {
+    CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D);
+    CXXRecordDecl *Class = MD->getParent();
----------------
Hmm, now we're adding an AST walk over all inline methods which probably slows us down a bit. Not sure I have any better ideas though.

In any case, ActOnFinishInlineFunctionDef needs a comment explaining why it's doing this.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5702
+            !getLangOpts().DllExportInlines &&
+            Class->getTemplateSpecializationKind() != TSK_ExplicitInstantiationDeclaration &&
+            Class->getTemplateSpecializationKind() != TSK_ExplicitInstantiationDefinition) {
----------------
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...


https://reviews.llvm.org/D51340