[PATCH] D128619: [Clang] Implement P0848 (Conditionally Trivial Special Member Functions)

[Roy Jacobson via Phabricator via cfe-commits]

royjacobson marked 4 inline comments as done and an inline comment as not done.
royjacobson added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:17960-17961
+        }
+        if (AnotherMethodIsMoreConstrained)
+          break;
+      }
----------------
cor3ntin wrote:
> Is that codepath ever taken?
> I wonder if there is a way to do that that is not n^2. Maybe not.
It's not, I didn't mean to leave it there. Thanks for the catch :)


Generally finding a maximal set in a partial ordering is O(n^2) in the worst case which is when no two objects are comparable.

We could optimize this a bit for some cases: We can group the functions by their kind, and we can skip comparing functions if we already know that they are not maximal. But since the number of SMFs expected in a class is very (1-3) small, I didn't think those possible improvements are worth the extra complexity.

Another possible optimization is we could short-circuit this evaluation if we know that a type is not trivially [copyable], since that's the only outcome we really care about, but then the AST is left in a very awkward state.



================
Comment at: clang/test/AST/conditionally-trivial-smfs.cpp:39
+
+template struct DefaultConstructorCheck<2>;
+// CHECK:             "kind": "ClassTemplateSpecializationDecl",
----------------
BRevzin wrote:
> It's possible that I just don't understand what these tests actually mean but... where is the test for `DefaultConstructorCheck<2>` having a deleted default constructor, or `DefaultConstructorCheck<3>` having a non-defaulted one?
> 
> It'd also be worthwhile to have at least one test with constaints that subsume each other instead of being mutually exclusive. 
Should I check this? Except destructors, the other SMFs are found during overload resolution using the usual lookup that already takes into account delete/default/constraints etc.

This patch is about making sure that we set the triviality attributes correctly according to the eligible functions, so this is what I added tests for.

Most of this testing is done in the sema test, but I need this AST test as well to make sure we get the `canPassInRegisters` attribute correctly - we're doing some custom processing over the functions without the usual type attributes because there are some weird compatibility edge cases.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128619/new/

https://reviews.llvm.org/D128619