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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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<
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108216