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: > > 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. 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
