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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Thanks! I don't think the new isThisDeclarationADefinition() and isImplicitlyInstantiable() checks are right. Besides that, I only have a few more comments about the test.



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5715
+          (MD->isThisDeclarationADefinition() ||
+           MD->isImplicitlyInstantiable()) &&
+          TSK != TSK_ExplicitInstantiationDeclaration &&
----------------
The isThisDeclarationADefinition() and isImplicitlyInstantiable() checks don't look right. Just checking isInlined() should be enough.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:9
+// RUN:     -fno-dllexport-inlines -emit-llvm -O1 -o - |                \
+// RUN:     FileCheck --check-prefix=STATIC %s
+
----------------
Why this separate STATIC invocation? It looks just the same as the invocation above.

Oh I see, it's because the test is mixing -DAG and -NOT lines, which breaks things.

I have a suggestion for avoiding the -NOT lines below, and that means this separate invocation to check for statics (and the other one below) can be removed.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:25
+
+  // NOEXPORTINLINE-NOT: ?InclassDefFunc at ExportedClass@@
+  // EXPORTINLINE-DAG: define weak_odr dso_local dllexport void @"?InclassDefFunc at ExportedClass@@
----------------
Instead of a -NOT check, it would be better to "use" the function somewhere so that it's emitted, and check that it's not dllexport. That will make it easier to see the difference between NOEXPORTINLINE and EXPORTINLINE.

Also it avoids -NOT checks, which don't interact well with -DAG checks, and would let you remove the separate STATIC invocation.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:44
+  // CHECK-NOT: InlineOutclassDefFuncWihtoutDefinition
+  inline void InlineOutclassDefFuncWihtoutDefinition();
+
----------------
Having an inline function and not defining it like this is not so great, especially in a dllexport class. I don't think there's any need to change the behaviour here or test for it.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:47
+  // CHECK-DAG:define weak_odr dso_local dllexport void @"?InlineOutclassDefFunc at ExportedClass@@QEAAXXZ
+  inline void InlineOutclassDefFunc();
+
----------------
But with -fno-dllexport-inlines, I don't think it should be exported.

It really doesn't matter if the body is inside or outside the class -- it's still an inline member function, and so the flag should apply.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:53
+  // CHECK-DAG: define dso_local dllexport void @"?OutclassDefFunc at ExportedClass@@QEAAXXZ"
+  void OutclassDefFunc();
+};
----------------
I'd suggest calling it "OutoflineDefFunc()" instead.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:65
+
+void __declspec(dllexport) ExportedClassUser() {
+  ExportedClass a;
----------------
I don't think there's any reason to dllexport the "user" function.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:81
+// EXPORTINLINE-DAG: define weak_odr dso_local dllexport void @"?InclassDefFunc@?$TemplateExportedClass at VA11@@@@AEAAXXZ"
+template class __declspec(dllexport) TemplateExportedClass<A11>;
+
----------------
I don't think this instantiation is different from the `​​template class TemplateExportedClass<B22>;` one below, so I think we can skip it.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:83
+
+// EXPORTINLINE-DAG: define weak_odr dso_local dllexport void @"?InclassDefFunc@?$TemplateExportedClass at VB22@@@@AEAAXXZ"
+template class TemplateExportedClass<B22>;
----------------
There should also be a check to make sure it's exported also in the NOEXPORTINLINE case.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:88
+// EXPORTINLINE-DAG: define weak_odr dso_local dllexport void @"?InclassDefFunc@?$TemplateExportedClass at VC33@@@@QEAAXXZ
+TemplateExportedClass<C33> c33;
+
----------------
Thanks, this is the implicit instantiation case. It would be good to have an inline function with a static local to make sure it does get exported even with the flag.


https://reviews.llvm.org/D51340