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

Wed Jul 20 14:21:46 PDT 2022

royjacobson added inline comments.

================
Comment at: clang/lib/Sema/SemaDecl.cpp:17899
+      ConstraintSatisfaction Satisfaction;
+      if (S.CheckFunctionConstraints(Method, Satisfaction))
+        SatisfactionStatus.push_back(false);
----------------
cor3ntin wrote:
> erichkeane wrote:
> > royjacobson wrote:
> > > erichkeane wrote:
> > > > royjacobson wrote:
> > > > > erichkeane wrote:
> > > > > > cor3ntin wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > cor3ntin wrote:
> > > > > > > > > royjacobson wrote:
> > > > > > > > > > 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?
> > > > > > > > > > 
> > > > > > > > > I think this is done on completeness already in this patch, unless i misunderstood the code.
> > > > > > > > > I don't think doing it on demand is a great direction, as this does not only affect type traits but also code gen, etc. It would create instanciations in unexpected places. wouldn't it.
> > > > > > > > > Does the standard has wording suggesting it should be done later than on type completeness?
> > > > > > > > The problem, at least for the deferred concepts, is that it breaks in the CRTP as required to implement ranges.  So something like this: https://godbolt.org/z/hPqrcqhx5 breaks.
> > > > > > > > 
> > > > > > > > I'm currently working to 'fix' that, so if this patch causes us to 'check' constraints early, it'll go back to breaking that example.  The example that Roy showed seems to show that it is actually checking 'delayed' right now (that is, on demand) in GCC/MSVC.  I don't know of the consequence/standardeeze that causes that though.
> > > > > > > Thanks, 
> > > > > > > Follow up stupid question then, do we care about the triviality of dependant types?
> > > > > > > I think doing the check on complete non-dependant types might be a better solution than trying to do it lazily on triviality checks?
> > > > > > I would think it would be not a problem on non-dependent types.  BUT concepts are only allowed on templated functions (note not only on function-templates!) anyway, so I don't think this would be a problem?  
> > > > > Erich, I'm a bit confused by your response. I think my example demonstrates that for default constructors (and other SMFs) GCC and MSVC instantiate the constraints on class completion and **not** on demand. This is what I would like to do as well, if we don't have a good reason not to. (For destructors, performing the checks is even explicit in the standard.)
> > > > > 
> > > > > Not doing this can introduce some REALLY bad edge cases. What does this do if we defer the triviality computation?
> > > > > 
> > > > > ```c++
> > > > > 
> > > > > template <class T>
> > > > > struct Base<T> {
> > > > >   Base() = default;
> > > > >   Base() requires (!std::is_trivial_v<T>);
> > > > > };
> > > > > 
> > > > > struct Child : Base<Child> { };
> > > > > ```
> > > > > We defer the computation of the constraints on `Base`, and complete `Child` somehow, but if `Child` is complete then `std::is_trivial_v<Child>` should be well-formed, right? But we get a logical contradiction instead.
> > > > > 
> > > > > 
> > > > >Erich, I'm a bit confused by your response
> > > > It is entirely possible we're talking past eachother, or misunderstanding eachothers examples.  I'm totally open to that being part of this issue.
> > > > 
> > > > In that example, if we calculate the triviality at '`Base` Class completion', `Child` is not yet complete, right?  So the is_trivial_v would be UB.  If you instead require `sizeof(T)`, we typically give a diagnostic.
> > > > 
> > > > In this case, you'd at MINIMUM have to wait to calculate the requires-clause until after `Child` is complete.  And it isn't clear to me that we're delaying it until then.
> > > > 
> > > > That is what I intend to show with: https://godbolt.org/z/1YjsdY73P
> > > > 
> > > > As far as doing it 'on demand', I definitely see your circular argument here, but I think the is_trivial_v test is UB if we calculate it at `Base' completion.
> > > I'm arguing for checking the constraints at the completion of `Base`, and for making `is_trivial_v/sizeof` ill formed in this context.
> > > 
> > > Your GCC example is a bit misleading, I think. They check the `sizeof(T) > 0` constraint at the completion of `Base` but they just swallow the error and declare the constraint unsatisfied or something. They should've probably issued a diagnostic or something. But if you look at which constructor they choose, they choose the unconstrained one: https://godbolt.org/z/rKj4q5Yx9
> > Hmm.  I think based on that example, I agree with you.  We can do the instantiation and mark it unviable at the end of `Base`.  I think I'm getting confused by Clang's lack of deferred instantiation in this part.
> > 
> > Thanks for talking this through with me!
> > 
> > I AM concerned about how this will work with my deferred instantiations, so a test that validates that (and has a FIXME that shows it is broken right now) would be appreciated.
> > I'm arguing for checking the constraints at the completion of `Base`, and for making `is_trivial_v/sizeof` ill formed in this context.
> > 
> > Your GCC example is a bit misleading, I think. They check the `sizeof(T) > 0` constraint at the completion of `Base` but they just swallow the error and declare the constraint unsatisfied or something. They should've probably issued a diagnostic or something. But if you look at which constructor they choose, they choose the unconstrained one: https://godbolt.org/z/rKj4q5Yx9
> 
> I don't think gcc is wrong per http://eel.is/c++draft/temp.constr.constr#temp.constr.op-5.sentence-3 
> The constrained overload is sfinea away silently, no diagnostic should be emitted.
> It's no different from https://godbolt.org/z/ThMPrhs93 , for example 
> 
I added the test, could you take a look and tell me if that's what you meant or if you want some changes? @erichkeane 
I hope it isn't disruptive to the deferred patch itself, it's a bit concerning indeed but I'm moderately optimistic :)

@cor3ntin thanks for the explanation!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128619