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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Thank you Takuto! I think the functionality looks great now: it's not too complicated :-)

I only have comments about the test.



================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:3
+// RUN:     -fno-dllexport-inlines -emit-llvm -O0 -o - |                \
+// RUN:     FileCheck --check-prefix=DEFAULT --check-prefix=NOINLINE %s
+
----------------
Instead of "DEFAULT", I think "CHECK" is the default prefix to use when using the same prefix for multiple invocations.

Also, instead of INLINE and NOINLINE (which sounds like it's about inlining), I'd suggest perhaps "EXPORTINLINES" and "NOEXPORTINLINES" or something like that.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:6
+// RUN: %clang_cc1 %s -fms-extensions -triple x86_64-windows-msvc       \
+// RUN:     -emit-llvm -O0 -o - |                                       \
+// RUN:     FileCheck --check-prefix=DEFAULT --check-prefix=INLINE %s
----------------
Instead of using -O0 here and above, consider using -O1 -disable-llvm-passes so we get available_externally functions.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:9
+
+// Function
+
----------------
This change doesn't have any effect on non-member functions, so I don't think we need these tests.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:42
+
+// check for local static variables
+// NOINLINE-DAG: @"?static_variable@?1??InclassDefFuncWithStaticVariable at NoTemplateExportedClass@@QEAAHXZ at 4HA" = weak_odr dso_local dllexport global i32 0, comdat, align 4
----------------
Can you move the checks down to where the variable/function is defined? That makes it easier to read the test I think.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:49
+
+class __declspec(dllexport) NoTemplateExportedClass {
+ public:
----------------
I think this test case should be first in the file, because it's really showing the core of this feature.

I would suggest just calling "ExportedClass" to make it simpler (if it was a template, then we'd put template in the name). Also, I suggest making it a struct instead so you don't have to write "public:".


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:51
+ public:
+  // DEFAULT-NOT: NoTemplateExportedClass at NoTemplateExportedClass@@
+  NoTemplateExportedClass() = default;
----------------
This check is only using part of the mangled name..
But I don't think the patch does anything special for this kind of function, so I'd just skip this test.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:58
+
+  int f();
+
----------------
This doesn't seem to be used for anything?


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:73
+  // DEFAULT-NOT: InlineOutclassDefFuncWihtoutDefinition
+  __forceinline void InlineOutclassDefFuncWihtoutDefinition();
+
----------------
Why __foceinline? I think it should be just "inline". This goes for all use of __forceinline in this file.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:76
+  // DEFAULT-NOT: InlineOutclassDefFunc at NoTemplateExportedClass@@
+  __forceinline void InlineOutclassDefFunc();
+
----------------
Hmm, I would have thought we should export this function.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:102
+  void InclassDefFunc() {}
+  void OutclassDefFunc();
+
----------------
I'm not sure that OutclassDefFunc() adds much value to the test. Also templateValue below. I think they can be removed.


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


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:132
+  }
+  void OutclassDefFunc();
+};
----------------
Again, I think we can drop the out-of-line function since this change should not affect those.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:144
+// DEFAULT-DAG: define weak_odr dso_local void @"?OutclassDefFunc@?$TemplateNoExportedClass at VB22@@@@QEAAXXZ"
+template class TemplateNoExportedClass<B22>;
+
----------------
This case is not interesting, since it has nothing to do with dllexport/import, I think it can be removed.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:148
+// DEFAULT-NOT: ?OutclassDefFunc@?$TemplateNoExportedClass at VC33@
+extern template class TemplateNoExportedClass<C33>;
+
----------------
Ditto.


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:165
+class __declspec(dllimport) ImportedClass {
+public:
+  class ImportedInnerClass {
----------------
Again, make the classes "struct" and remove the "public:" lines :-)


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:166
+public:
+  class ImportedInnerClass {
+   public:
----------------
I don't see any test lines that reference this inner class. I guess it's not interesting?


================
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() {
----------------
This check looks wrong. I think there should be a dllimport declaration for this function? (Or an available_externally definition if we use -O1?)


================
Comment at: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp:181
+  // INLINE-DAG: declare dllimport void @"?InClassDefFunc at ImportedClass@@QEAAXXZ"
+  void InClassDefFunc() {
+    i.OutClassDefFunc();
----------------
Let's put this first in the class, since it's the "basic" case, and the static local is more of a special case.


https://reviews.llvm.org/D51340