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



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

Reply via email to