boostorg / core

Boost Core Utilities
133 stars 83 forks source link

add void_t to core #124

Closed HDembinski closed 1 year ago

HDembinski commented 2 years ago

The following variadic template exists in hana, beast, json, describe, math, histogram, mp11, and probably many other boost libraries:

template <typename ...>
using void_t = void;

Please add it to core. I can draft a PR if this is desired.

Lastique commented 2 years ago

Why not Boost.TypeTraits?

pdimov commented 2 years ago

It's already there. https://github.com/boostorg/type_traits/blob/develop/include/boost/type_traits/make_void.hpp

Lastique commented 2 years ago

Then I suppose we can close this.

HDembinski commented 2 years ago

Please reopen. Core is supposed to provide basic types used throughout Boost. It exists so that we can reduce internal coupling of Boost libraries. Boost Histogram is not using Boost type_traits, it is using std type_traits for that reason. Hence I cannot import void_t from there, it has to be in core.

Lastique commented 2 years ago

On October 2, 2022 1:42:42 PM Hans Dembinski @.***> wrote:

Please reopen. Core is supposed to provide basic types used throughout Boost. It exists so that we do not have to reduce internal coupling of Boost libraries. Boost Histogram is not using Boost type_traits, it is using std type_traits. Hence I cannot import void_t from there.

I don't think this is a valid reason to duplicate code in Core. We'll end up with everything in Core this way.

If you don't want to rely on Boost.TypeTraits then you'll have to implement void_t in Histogram. Gladly, it's not difficult. But frankly, I don't see the problem. You're already ok to depend on Core, and TypeTraits doesn't add more dependencies, IIRC.

pdimov commented 2 years ago

I agree that we shouldn't be duplicating TypeTraits in Core. TypeTraits used to be a part of a dependency cycle with MPL, Utility, and other stuff, but these days are long past and it only depends on Config and StaticAssert nowadays.

HDembinski commented 2 years ago

I don't want type_traits in core, just void_t. Having one dependency less for Histogram is better than having one more. It is simple. Yes, I can and have implemented void_t myself, but we have types in core that are equally trivial, boost::type for instance.

HDembinski commented 2 years ago

Perhaps you could formulate the rules precisely for what should be adopted in core and what not, because I am getting the impression that it is arbitrary. Please reopen.

pdimov commented 2 years ago

boost::type is a legacy, from the time we moved things into Core in order to break library cycles. enable_if exists here for the same reason. Now that TypeTraits has enable_if, I even prefer to use that instead of the Core one.

pdimov commented 2 years ago

If you so insist on having void_t in Core, please do a PR (with documentation and tests) and then argue your case on the list. I'm not going to switch either Describe or Mp11 to it, and Math is not going to switch either, but the rest of the libraries might.

Lastique commented 2 years ago

Perhaps you could formulate the rules precisely for what should be adopted in core and what not, because I am getting the impression that it is arbitrary.

From the start, Core was Utility, only with less dependencies - this was one of the reasons Core was created. This is about as formal as you'll get. Yes, inclusion is somewhat arbitrary, but there should be strong reasons for the component to be in Core and not somewhere else, especially when there is a library where the component fits perfectly, doubly so when it already exists there. IMO, dropping exactly one dependency is not that reason. I could understand if the choice was between having zero Boost dependencies and one, but that's not the case (and having void_t in Core wouldn't have solved it anyway).

You want void_t, someone else wants is_same (and despite my protest it was added), someone third will want remove_cv or remove_reference because these are universally used. Then someone wants unique_ptr, shared_ptr and whatnot because those are basic vocabulary types, right? You see where this is going? I'm strongly against this. We have dedicated libraries for this, use those. Deal with dependencies. Or don't, and implement everything you need locally.

And for the record, I still think boost::core::is_same was a bad idea. I would like it to be removed or hidden in detail. You want is_same - use the one from TypeTraits, there should be none in Core (none public, anyway).

I didn't know we added enable_if_ in TypeTraits. This, again, perpetuates the mess. We should deprecate one (apparently, the one in Core at this point) and guide everyone to TypeTraits. I would also like us to rename it to the standard enable_if.

pdimov commented 2 years ago

Yeah, the only reason we have is_same in Core is because it's needed by lwtest, and Core was/is not allowed to depend on TypeTraits, so I had to duplicate is_same here.

Lastique commented 2 years ago

Can it be moved to detail? Both namespace and directory.

pdimov commented 2 years ago

Yes in principle, but it will break a lot of stuff now.

Lastique commented 2 years ago

We can mark it as deprecated, with warnings. Then remove the deprecated name and header after a couple Boost releases.

vinniefalco commented 1 year ago

Here is a metric. C++11 libraries should largely not have to include Boost.TypeTraits. I have a handful of C++11 libraries that only require void_t.

Lastique commented 1 year ago

C++ version is irrelevant. You include whatever library you need. If you need something from TypeTraits that is not present in the standard library, you include TypeTraits. The fact that the thing you need is a one-liner is not a ground to pull that one-liner to Core.

vinniefalco commented 1 year ago

My point is that the chances of needing something from TypeTraits other than void_t goes down dramatically in C++11 and later.

pdimov commented 1 year ago

If you don't need remove_cvref_t are you writing code at all? There's also is_detected and friends. Also, all the _t things are C++14 in <type_traits>.

vinniefalco commented 1 year ago

I have a close relationship with typename

glenfe commented 1 year ago

By the time we add several hundred traits to Core for this reason, people will complain that Core is too heavy.

Lastique commented 1 year ago

@pdimov Can we close this? I've deprecated is_same, and we (Boost.Core maintainers) seem to agree we don't want to duplicate TypeTraits in Core.

HDembinski commented 1 year ago

What an unreasonable discussion. You went from my request of adding void_t, a one-liner to "duplicate TypeTraits in Core" and "add several hundred traits"? This is not a slippery slope that you open up here.

glenfe commented 1 year ago

Aside from enable_if (now that we're deprecating is_same, and now that people are switching to Core's allocator_traits) is there anything in Core that is a duplicate of another facility in Boost?

vinniefalco commented 1 year ago

On the flip side, it is easy enough for me to just add this to <boost/buffers/detail/config.hpp>:

// avoid all of Boost.Types for just this
template<class...> struct make_void { typedef void type; };
template<class... Ts> using void_t = typename make_void<Ts...>::type;

its just 3 lines

Lastique commented 1 year ago

Aside from enable_if (now that we're deprecating is_same, and now that people are switching to Core's allocator_traits) is there anything in Core that is a duplicate of another facility in Boost?

glenfe commented 1 year ago

Why?

I was going to declare that we only moved existing components into Core, not duplicated them here, but I wasn't sure.