[PATCH] D90849: [dllexport] Avoid multiple codegen assert for explicitly defaulted methods in explicit instantiation definitions (PR47683)

[Reid Kleckner via Phabricator via cfe-commits]

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