CaseyCarter added inline comments.
================ Comment at: include/concepts:175 +template <class _Tp, class _Up> +_LIBCPP_CONCEPT_DECL Same = __same_impl<_Tp, _Up> && __same_impl<_Up, _Tp>; + ---------------- Quuxplusone wrote: > Peanut gallery asks: From lines 166-171 it looks awfully like > `__same_impl<_Tp, _Up>` is true if and only if `__same_impl<_Up, _Tp>` is > true. So why bother instantiating both templates? This is the library implementation of the ["`Same<T, U>` subsumes `Same<U, T>`" requirement](http://eel.is/c++draft/concept.same#1). ================ Comment at: include/concepts:280 + Constructible<_Tp, const _Tp&> && ConvertibleTo<const _Tp&, _Tp> && + Constructible<_Tp, const _Tp> && ConvertibleTo<const _Tp, _Tp>; + ---------------- Quuxplusone wrote: > Peanut gallery notices the lack of `Constructible<_Tp, const _Tp&&>` here, > and assumes it's intentional? Yes: `Constructible<T, Args..>` is equal to `is_constructible_v<T, Args...>` which is true iff the declaration `T t(declval<Args>()...);` is valid. Since `decltype(declval<T>())` is `add_rvalue_reference_t<T>`, `Constructible<_Tp, const _Tp>` is equal to `Constructible<_Tp, const _Tp&&>`. Admittedly the compiler doesn't know that - the subsumption rules don't know how `declval` is implemented - but no one has yet presented a use case where it would be helpful. If and when that happens, we can consider adding more terms to the concepts. ================ Comment at: include/concepts:295 + requires ConvertibleTo<const remove_reference_t<_Tp>&, bool>; + !__t1; requires ConvertibleTo<decltype(!__t1), bool>; + __t1 && __t2; requires Same<decltype(__t1 && __t2), bool>; ---------------- Quuxplusone wrote: > Peanut gallery asks: Am I correct that the instances of `ConvertibleTo<..., > bool>` here could (according to the WD) be replaced with `-> bool`? Am I > correct that the instances of `Same<..., bool>` here could eventually be > replaced with `-> Same<bool>` once they fix that issue you wrote a paper > about? > > I assume they're written like this for portability reasons. `ConvertibleTo` requires both implicit and explicit conversions to be valid and have equal results - so it's stronger than `is_convertible` and therefore the core language's notion of implicit convertibility. Consequently `-> bool` would not be equivalent. Yes, P1084 will let us simplify all of these horrible nested requirements that duplicate the expressions. `E; requires Concept<decltype(E), Args..>;` will become `{ E } -> Concept<Args...>;`. ================ Comment at: include/concepts:379 +_LIBCPP_CONCEPT_DECL Invocable = requires(_Fn&& __fn, _Args&&... __args) { + __invoke_constexpr(static_cast<_Fn&&>(__fn), static_cast<_Args&&>(__args)...); +}; ---------------- Quuxplusone wrote: > Does this need a `_VSTD::` for ADL reasons? (There's one in `<tuple>` that > does use `_VSTD::`; there's two in `<variant>` that don't.) Yes it does. Despite that the function name is ugly and a user can't hijack it, ADL will do crazy things like instantiating `T` when passed `T*` to determine `T`'s associated namespaces. ================ Comment at: include/concepts:394 + CommonReference< + const remove_reference_t<_Tp>&, + const remove_reference_t<_Up>&> && ---------------- Quuxplusone wrote: > I'm fairly confident that `const remove_reference_t<_Tp>&` is just `const > _Tp&`. Not when `_Tp` is a reference type: https://godbolt.org/g/Q2Hon8. https://reviews.llvm.org/D49120 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
