[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:
> 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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108216