[libcxx-commits] [PATCH] D108216: [libc++] Formulate _EnableIf in terms of std::enable_if

Wed Aug 18 07:22:53 PDT 2021

Quuxplusone added inline comments.

================
Comment at: libcxx/include/__functional/bind_front.h:41

-template <class _Fn, class... _Args, class = _EnableIf<
+template <class _Fn, class... _Args, class = enable_if_t<
     _And<
----------------
ldionne wrote:
> Quuxplusone wrote:
> > (1) What do the respective error messages look like with GCC? MSVC? (I bet those compilers lack Clang's special cases.)
> > (2) Is it conceivable that we could just make Clang understand anything-of-the-general-form-of-`_EnableIf`, based on the class template's shape, instead of based on special-casing its name?
> > (3) If all else fails, could we keep using the spelling `_EnableIf`, and just define `_EnableIf` as an alias for `enable_if_t`? At least then it would be easy for anyone who cared about compile-time performance to just patch it in one place. (Arguably I bet it would do no harm for them to patch it at the `enable_if_t` level, and thus speed up even more things than just libc++ itself.) Also, it would make this patch a lot less invasive.
> > (4) Is this the camel's nose in the tent? Are you soon going to propose replacing `_If` with `conditional_t`? Do we want this?
> 
> > (1) What do the respective error messages look like with GCC? MSVC? (I bet those compilers lack Clang's special cases.)
> 
> Indeed, only Clang provides a better diagnostic (but I still think it matters because Clang is by far the most important compiler for libc++. https://godbolt.org/z/4jx35WcrP
> 
> > (2) Is it conceivable that we could just make Clang understand anything-of-the-general-form-of-`_EnableIf`, based on the class template's shape, instead of based on special-casing its name?
> 
> I think we could, although we might end up with false positives. Would you consider any template that doesn't have a nested `::type` and has a `template <bool, class = void>` parameter list to be an `enable_if`? See https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplate.cpp#L10483.
> 
> > (3) If all else fails, could we keep using the spelling `_EnableIf`, and just define `_EnableIf` as an alias for `enable_if_t`? At least then it would be easy for anyone who cared about compile-time performance to just patch it in one place.
> 
> I guess the question I have here is "why do you prefer `_EnableIf` over `enable_if_t`? My mental model is that we strive to use standard utilities when we can, and we use shims like `_EnableIf` (which should really be called `__enable_if_t`) when we need the utility in C++N but it's not standardized until some later C++M version. Just like we have e.g. `__to_address` for pre-C++20 code. I think it would be good to agree on this general question.
> 
> > (4) Is this the camel's nose in the tent? Are you soon going to propose replacing _If with conditional_t? Do we want this?
> 
> I had no plans to do that since I'm not aware of a tangible benefit, however you're right that depending on what we end up agreeing upon for (3), the logical successor to this change could be to also move to `conditional_t` (whether we'll actually do it is a different story).
> 
> Note: I thought about defining `enable_if_t` the same way `_EnableIf` is defined for compile-time performance, however Clang apparently doesn't understand that either: https://godbolt.org/z/jE5T7GxPb. I do think this one would be easier to fix, though. (I'm surprised it doesn't work given this: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplate.cpp#L3514)
> (2) Would you consider any template that doesn't have a nested `::type` and has a `template <bool, class = void>` parameter list to be an `enable_if`?
Well, remember that our `_EnableIf` has a different shape from the standard `enable_if_t`. Ours is `_MetaBase<B>::_EnableIfImpl<R>`, whereas `enable_if_t` is `enable_if<B, R>::type`. So I think the criterion would be something like "We failed to find a member in a class template of the form `template<[...], bool, [...]>`, where that bool parameter is `false`, //and// where, if the bool parameter had been `true`, we //would// have found such a member." If that's too backtracky (Clang sucks at backtracking/hypotheticals), we could say "We failed to find a member in a class template of the form `template<[...], bool, [...]>`, where that bool parameter is `false`, //and// where the class template has at least one partial or explicit specialization." I would //not// look at the name of the member, nor the name of the class, nor the reason-we're-looking-for-that-member (e.g. the name of the alias template).

> Note: I thought about defining `enable_if_t` the same way `_EnableIf` is defined for compile-time performance
That would be non-conforming (and also an ABI break). If it were conforming, we'd have done it long ago. Here's why: the alias's expansion is observable by the programmer. https://godbolt.org/z/YeaY6fvWW

Re `isEnableIfAliasTemplate`: Again, the problem is that Clang hard-codes the names of these entities. Look at https://godbolt.org/z/W5Yf6qGM6 — change `E` from `enable_if` to `enable_if2`, change `ET` from `enable_if_t` to `enable_if_t2`, and observe how the diagnostics change their wording in response to the various name-changes. I think this is bad of Clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108216