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

[Takuto Ikuta via Phabricator via cfe-commits]

takuto.ikuta added a comment.

Hans, thank you for review! I addressed all your comment and fixed small behavior.



================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:76
+  // DEFAULT-NOT: InlineOutclassDefFunc at NoTemplateExportedClass@@
+  __forceinline void InlineOutclassDefFunc();
+
----------------
hans wrote:
> Hmm, I would have thought we should export this function.
I see.
Added  `MD->isThisDeclarationADefinition() ` check in `Sema::checkClassLevelDLLAttribute`
This will make change like https://chromium-review.googlesource.com/c/v8/v8/+/1186017 unnecessary.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:122
+template class TemplateExportedClass<B22>;
+
+
----------------
hans wrote:
> We should also have a test with implicit instantiation, and then the inline function should not be exported when using /Zc:dllexportInlines-.
Added test and `MD->isImplicitlyInstantiable()` check for the test.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:172
+  // INLINE-DAG: declare dllimport i32 @"?InClassDefFuncWithStaticVariable at ImportedClass@@QEAAHXZ"(%class.ImportedClass*) #2
+  // NOINLINE-NOT: declare{{.*}}"?InClassDefFuncWithStaticVariable at ImportedClass@@QEAAHXZ"
+  int InClassDefFuncWithStaticVariable() {
----------------
hans wrote:
> This check looks wrong. I think there should be a dllimport declaration for this function? (Or an available_externally definition if we use -O1?)
I think it is OK not dllimporting this function as far as static variable is dllimported and definition of this function is emitted.



https://reviews.llvm.org/D51340