[PATCH] D90849: [dllexport] Avoid multiple codegen assert for explicitly defaulted methods in explicit instantiation definitions (PR47683)
Reid Kleckner via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 5 09:27:53 PST 2020
rnk added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5887-5915
if (MD->isUserProvided()) {
// Instantiate non-default class member functions ...
// .. except for certain kinds of template specializations.
if (TSK == TSK_ImplicitInstantiation && !ClassAttr->isInherited())
continue;
----------------
I wonder if this code block could be simplified by calculating two booleans:
- ShouldReference: If true, call S.MarkFunctionReferenced
- ShouldHandleNow: if true, call HandleTopLevelDecl
We want to mark the vast majority of C++ method decls referenced. The conditions I see here are:
- user provided
- non-trivial
- trivial explicitly defaulted
- trivial copy assign & move assign
- an exception for non-inherited attributes on implicit specializations (?)
- an exception for defaulted explicit instantiations
I tried to come up with a simpler description of those conditions, and couldn't come up with one.
Nevermind. I don't think my suggestion will make the code any simpler, but I'll leave this as a comment for your consideration.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5898
// encountered.
} else if (!MD->isTrivial() || MD->isExplicitlyDefaulted() ||
MD->isCopyAssignmentOperator() ||
----------------
Part of me wants to handle explicitly defaulted things separately from implicit special members, so we don't have to check for explicitly defaulted-ness twice.
================
Comment at: clang/test/CodeGenCXX/dllexport.cpp:929
+template <typename> struct T {
+ T() = default;
+ X x;
----------------
Let's also add a test case where the constructor is explicitly defaulted outside the body of the class. From reading the code, I know we will take the isUserProvided path instead, but I'd like to have it for completeness.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90849/new/
https://reviews.llvm.org/D90849
More information about the cfe-commits
mailing list