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