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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
