[libcxx-commits] [PATCH] D108216: [libc++] Formulate _EnableIf in terms of std::enable_if
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Sep 7 18:35:50 PDT 2021
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:
> ldionne wrote:
> > Quuxplusone wrote:
> > > Quuxplusone wrote:
> > > > ldionne wrote:
> > > > > Quuxplusone wrote:
> > > > > > 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?
> > > > > I applied this patch so that we only compile everything in the test suite:
> > > > >
> > > > > ```
> > > > > diff --git a/libcxx/utils/libcxx/test/format.py b/libcxx/utils/libcxx/test/format.py
> > > > > index e4e2937f0a5a..064db5627677 100644
> > > > > --- a/libcxx/utils/libcxx/test/format.py
> > > > > +++ b/libcxx/utils/libcxx/test/format.py
> > > > > @@ -249,7 +249,6 @@ class CxxStandardLibraryTest(lit.formats.TestFormat):
> > > > > elif filename.endswith('.link.fail.cpp'):
> > > > > steps = [
> > > > > "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} -c -o %t.o",
> > > > > - "%dbg(LINKED WITH) ! %{cxx} %t.o %{flags} %{link_flags} -o %t.exe"
> > > > > ]
> > > > > return self._executeShTest(test, litConfig, steps)
> > > > > elif filename.endswith('.verify.cpp'):
> > > > > @@ -267,7 +266,6 @@ class CxxStandardLibraryTest(lit.formats.TestFormat):
> > > > > elif filename.endswith('.pass.cpp') or filename.endswith('.pass.mm'):
> > > > > steps = [
> > > > > "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} %{link_flags} -o %t.exe",
> > > > > - "%dbg(EXECUTED AS) %{exec} %t.exe"
> > > > > ]
> > > > > return self._executeShTest(test, litConfig, steps)
> > > > > # This is like a .verify.cpp test when clang-verify is supported,
> > > > > ```
> > > > >
> > > > > This is what I got:
> > > > >
> > > > > With enable_if_t (my patch)
> > > > > ---------------------------
> > > > >
> > > > > ```
> > > > > ninja -C build/default check-cxx 5509.93s user 987.85s system 1737% cpu 6:13.89 total
> > > > > ninja -C build/default check-cxx 5513.07s user 991.30s system 1866% cpu 5:48.45 total
> > > > > ```
> > > > >
> > > > > With _EnableIf (status quo)
> > > > > ---------------------------
> > > > >
> > > > > ```
> > > > > ninja -C build/default check-cxx 5508.53s user 1069.88s system 1876% cpu 5:50.60 total
> > > > > ninja -C build/default check-cxx 5514.48s user 1000.38s system 1874% cpu 5:47.62 total
> > > > > ```
> > > > >
> > > > > From the test suite at least, I'm not seeing any significant difference. That may not be the best benchmark though.
> > > > >
> > > > I've filed https://bugs.llvm.org/show_bug.cgi?id=51696 for the Clang diagnostic, not that I expect any movement on it anytime soon.
> > > I've continued down the rabbit hole far enough for a blog post:
> > > https://quuxplusone.github.io/blog/2021/09/04/enable-if-benchmark/
> > > Also relevant to D109212.
> > Awesome, thanks a lot for looking into this!
> >
> > So, do we agree that the discussion about whether to use `_EnableIf` or `enable_if_t` is moot now, and we can go forward with this patch since it makes diagnostics better?
> >
> > As a follow-up, we could change instances of `class = enable_if_t<...>` into `enable_if_t<...>* = nullptr` to get a slight improvement on GCC. WDYT?
> Well, as per my comment of 2021-09-01, I'd //still// prefer to bottleneck `_EnableIf` — and in fact I'd even prefer to change all existing instances of `enable_if` into `_EnableIf`, //except// where `enable_if` is required by law — to make it clear that we control these implementation details (//except// where `enable_if` is required by law).
> However, I admit that (given the current state of Clang's diagnostic engine) that would negate the benefit-in-error-messages you're trying to achieve. So we're at an impasse, and I suppose you win by default, if you want `enable_if` badly enough; I won't be terribly upset, I'll just maybe get to say "I told you so" in five years when we have to touch them all again. ;)
> I agree that I see nothing physically wrong with your suggestion to change `class = _EnableIf<...>` into `_EnableIf<...>* = nullptr` (or the equivalent with `enable_if_t`). (Messing with template parameter kinds that way does break ABI, but all of these templates have inline visibility, so we are cool with the break.)
>
> Actually, isn't there still a logistical question: Are there any `_EnableIf`s in C++03/11 codepaths? You can't replace those with `_VSTD::enable_if_t` because it doesn't exist until C++14. So what: leave them as `_EnableIf`? use `typename enable_if<...>::type`? introduce a new `__enable_if_t` for portable use (which, cough cough, is just `_EnableIf` renamed)?
> (except where `enable_if` is required by law).
Are there any such places? I'm genuinely asking, cause I don't think I've noticed any. Usually the standard says "constraints" and that means "`enable_if` or whatever SFINAE method you want", but I don't think I've seen any places where `enable_if` exactly is required (except in the definition of `enable_if_t`).
> So we're at an impasse, and I suppose you win by default, if you want `enable_if` badly enough;
As I said, what I want is to improve the terrible state of our diagnostics today (seriously, they suck pretty bad and the introduction of `_EnableIf` can be seen as a regression in that regard, which I'm trying to fix a couple years too late). I don't want to gratuitously override you, which is why I'm trying to ask whether you agree that we're at a dead end -- I'd be OK with keeping `_EnableIf` if we could get nice diagnostics, but we can't, so it seems like this patch (pretty much as-is) is the only way forward that gets us decent diagnostics.
> Messing with template parameter kinds that way does break ABI, but all of these templates have inline visibility, so we are cool with the break.
Yes, that is my understanding too.
> Are there any `_EnableIfs` in C++03/11 codepaths? You can't replace those with `_VSTD::enable_if_t` because it doesn't exist until C++14. So what: leave them as `_EnableIf`?
Yes. The intent of this patch was to only touch code paths that were in a Standard mode where `enable_if_t` is defined by the library, i.e. only C++14-and-above code paths. The code paths where `enable_if_t` isn't an option were left to use `_EnableIf`.
> use `typename enable_if<...>::type`? introduce a new `__enable_if_t` for portable use (which, cough cough, is just `_EnableIf` renamed)?
I left those code paths to use `_EnableIf` to avoid touching them, but I agree that a more consistent approach would be to use `__enable_if_t` -- and I'd perform such a renaming as a follow-up NFC patch afterwards, I think.
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