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

Arthur O'Dwyer via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Aug 30 14:25:20 PDT 2021 

Quuxplusone added a subscriber: EricWF.
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:
> > ldionne wrote:
> > > Quuxplusone wrote:
> > > > 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.
> > > Concretely, I don't think trying to guess that a class is a `enable_if`-like thing based on its shape is a good idea. It's too complicated and can have false positives. If we were to do something, I'd do something dumb like add `_EnableIf` to the list of things we look for, or something very constrained along those lines.
> > > 
> > > > Here's why: the alias's expansion is observable by the programmer. https://godbolt.org/z/YeaY6fvWW
> > > 
> > > Ah, geez, indeed.
> > > 
> > > 
> > > I think what we need to settle on is this:
> > > 
> > > > 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.
> > > 
> > > Concretely, I don't think trying to guess that a class is a `enable_if`-like thing based on its shape is a good idea. It's too complicated and can have false positives.
> > 
> > I think it's a great idea and there's no such thing as a false positive (since the diagnostic is in the form of an additional explanatory note that specifically //adds// information about the boolean expression that evaluated to `false` instead of `true`)... but that disagreement may be moot, since I don't think anyone's stepping up to actually write that improved diagnostic anytime soon.
> > 
> > > 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 when we need the utility in C++N but it's not standardized until some later version.
> > 
> > I tend to see everything in terms of "maintainability bottlenecks" (there must be a standard term for this, but I'm sticking with "bottleneck" for the purposes of this message). Whenever we use a third-party dependency — whether it's Boost or PCRE or std::regex or std::enable_if_t — we have a choice of using it //directly// in our codebase (scattering its uses all over), or else piping it through a deliberately constructed bottleneck where we can easily swap it out for something else. For some things, like `std::regex`, we obviously want to bottleneck it, because the dependency sucks so badly that we //know// we'll want to replace it later. For some things like `std::string`, we obviously //don't// want to bottleneck it. In an average industry codebase, we don't care to bottleneck `std::enable_if_t` either, because it's not a hotspot; but for libc++, where it's used super heavily, and where we //know// a better replacement, I think it's reasonable to bottleneck it the way Eric did.
> > 
> > You're right that sometimes we bottleneck things because we need to conditionally replace them with a polyfill (like how codebases will have a namespace alias `fs` which is either `std::filesystem` or `boost::filesystem`; and I recently did the same thing with `namespace asio`) but polyfilling is not the //only// reason we've ever bottlenecked something.
> I understand exactly what you mean (the term I use in my head when I think about those is "indirection points", but I guess everyone has their personal way of thinking about it).
> 
> Concretely, do we have a suggestion where we keep using `_EnableIf` but also get better diagnostics than the terrible ones we get today? If not, I think this patch is pretty important. Also, I'd be curious to see what is the actual benefit of using `_EnableIf` the way we have it today -- it's not clear to me the compile-time cost comes from instantiating `enable_if<cond, void>` more than it comes from evaluating any expression that forms the `cond`.
> Concretely, do we have a suggestion where we keep using _EnableIf but also get better diagnostics than the terrible ones we get today? If not, I think this patch is pretty important.

I just tried my suggestion of aliasing `_EnableIf` to `enable_if_t<_Bp, _Tp>`. It turns out that Clang's diagnostic is already not even //that// smart: it loses the spelling of the boolean condition.
https://godbolt.org/z/eWoe9M986
```
<source>:13:36: note: candidate template ignored: requirement 'false' was not satisfied [with _Fn = Evil &]
_LIBCPP_CONSTEXPR_AFTER_CXX17 auto not_fn1(_Fn&& __f) {
                                   ^
```
(That Godbolt has `std::not_fn` with the current technique, `std::not_fn1` with the aliasing technique, and `std::not_fn2` with your proposed direct use of `enable_if_t`.)

I think it'd be worth pursuing the Clang patch ("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"), but concretely, no, I'm not planning to pursue it myself.

> Also, I'd be curious to see what is the actual benefit

So would I. Do you think it would be useful to run (some part of) the test suite (in compile-only mode, if such a thing exists) in both configurations, and see if there's any detectable difference in how long it takes? I tried just now with HyperRogue, but of course there was no detectable difference. To see a difference you'd have to be doing a lot of instantiations of enable-iffed libc++ components, which is already a difficult achievement.

Then I went further into the rabbit hole with this Python script to generate .cpp files to test:
```
print("""
#include <type_traits>
template<int N> struct priority_tag : priority_tag<N-1> {};
template<> struct priority_tag<0> {};
template<int N> struct A {};
template<bool> struct MetaBase {};
template<> struct MetaBase<true> { template<class T> using If = T; };
template<bool B, class T> using EnableIf = typename MetaBase<B>::template If<T>;
""")

# OPTION NUMBER ONE
for i in range(200):
    print("template<int N> auto f(priority_tag<%d>) -> std::enable_if_t<N==%d, A<%d>>;" % (i, i, i))

# OPTION NUMBER TWO
#for i in range(200):
#    print("template<int N> auto f(priority_tag<%d>) -> EnableIf<N==%d, A<%d>>;" % (i, i, i))

# OPTION NUMBER THREE
#for i in range(200):
#    print("template<int N> auto f(priority_tag<%d>) -> A<%d> requires (N==%d);" % (i, i, i))

print("void test() {")
for i in range(200):
    print("f<%d>(priority_tag<200>{});" % i)
print("}")
```
But still this produced no significant results. (~2.5 seconds to compile options 1 and 2; ~3.0 seconds to compile option 3. I'm surprised by that Concepts slowdown, but my numbers are also //very// noisy and might be completely wrong.)

`_EnableIf` seems to have happened rather quickly in 51a741c87fa8f87c8b37015dac4bce2b10d1ce81 (2019-06-21) and then 3359a17b3aef1effa494da3abe7f438f5bb184a7 (2019-06-23) — no Differentials, no links to benchmarks in the commit messages. Maybe it's //not// justified on performance grounds. @EricWF, any thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108216

More information about the libcxx-commits mailing list