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

[Roy Jacobson via Phabricator via cfe-commits]

royjacobson added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:17870
+///   parameter type and the same cv-qualifiers and ref-qualifier, if any.
+bool AreSpecialMemberFunctionsSameKind(CXXMethodDecl *M1, CXXMethodDecl *M2,
+                     Sema::CXXSpecialMember CSM) {
----------------
cor3ntin wrote:
> 
It's in an anonymous namespace. Is it a Clang convention to use static functions instead? I saw both used in Sema, I think


================
Comment at: clang/lib/Sema/SemaDecl.cpp:17874
+      return true;
+    if (M1->getParamDecl(0)->getType() != M2->getParamDecl(0)->getType())
+      return false;
----------------
erichkeane wrote:
> I don't think you'd want to compare with equality here, it ends up not desugaring/etc.  I believe `ASTContext::HasSameType` should be what you want.  
Good catch! added a test for this.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:17899
+      ConstraintSatisfaction Satisfaction;
+      if (S.CheckFunctionConstraints(Method, Satisfaction))
+        SatisfactionStatus.push_back(false);
----------------
erichkeane wrote:
> This seems problematic, doesn't it?  Checking this constraint will (once I figure out how to get deferred instantiation to work) cause instantiation, which can cause issues with incomplete types/CRTP/etc.
> 
> I think the result is that we cannot 'calculate' this until it is queried, else we will cause incorrect errors.
Making this queried on demand is a relatively big change to how we handle type triviality, so I want to be sure we actually need to do this to be conformant.

When I started working on this I checked what GCC does and it instantiates those constraints during class completion as well. For example this CRTP case: https://godbolt.org/z/EdoYf96zq. MSVC seem to do it as well.

So maybe it's OK to check the constraints of SMFs specifically?



================
Comment at: clang/lib/Sema/SemaDecl.cpp:17960-17961
+        }
+        if (AnotherMethodIsMoreConstrained)
+          break;
+      }
----------------
cor3ntin wrote:
> royjacobson wrote:
> > 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.
> > 
> Thanks, I guess this is fine, I cannot really think of a better way either
Ok, did another look on this and it's because `AnotherMethodIsMoreConstrained` is the output of `IsAtLeastAsConstrained`, so that's why the extra check. Added it back :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128619