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@ExportedClass@@
+ // EXPORTINLINE-DAG: define weak_odr dso_local dllexport void
@"?InclassDefFunc@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@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@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@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@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@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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
