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@NoTemplateExportedClass@@QEAAHXZ@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@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@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@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@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@ImportedClass@@QEAAHXZ"(%class.ImportedClass*)
#2
+ // NOINLINE-NOT:
declare{{.*}}"?InClassDefFuncWithStaticVariable@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@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
