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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:17899
+      ConstraintSatisfaction Satisfaction;
+      if (S.CheckFunctionConstraints(Method, Satisfaction))
+        SatisfactionStatus.push_back(false);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128619