Factory method for cuco::static_set #297
Conversation
|
Marking this as a draft since there are several details I'd like to discuss in the reviews. Also, some docs are still missing. |
| cuco::experimental::static_set<Key> set{ | ||
| size, cuco::empty_key<Key>{-1}, {}, {}, {}, {launch.get_stream()}}; | ||
| auto set = cuco::experimental::make_static_set<Key>( | ||
| cuco::experimental::extent<std::size_t>{size}, | ||
| cuco::empty_key<Key>{-1}, | ||
| cuco::experimental::cuda_stream_ref{launch.get_stream()}); |
There was a problem hiding this comment.
Using the factory doesn't save you any characters (at least until we remove the experimental namespace). However, not having to remember the exact order of arguments is a huge win in my UI book.
There was a problem hiding this comment.
I assume size and empty sentinel are necessary and all others are optional?
There was a problem hiding this comment.
Yep. Also, for the tparams, only the Key is necessary.
| Extent capacity, | ||
| empty_key<Key> empty_key_sentinel, | ||
| KeyEqual pred, | ||
| ProbingScheme const& probing_scheme, | ||
| Allocator const& alloc, | ||
| [[maybe_unused]] Storage const& storage, |
There was a problem hiding this comment.
I don't like this. We're using storage as a dummy object here to deduce the Storage type from. A user might misinterpret that as an opportunity to pass in an existing storage for the container to use.
There was a problem hiding this comment.
I have a solution in mind that eliminates the storage argument from the ctor. I'm still fiddling with it a bit.
| class Extent = cuco::experimental::extent<std::size_t>, | ||
| cuda::thread_scope Scope = cuda::thread_scope_device, | ||
| class Extent = cuco::experimental::extent<std::size_t>, |
There was a problem hiding this comment.
Since cuda::thread_scope is an enum type, for it to be available at compile time, we need to pass it in the template parameter list of the factory instead of the runtime argument list:
make_static_set<Key>(cuda::thread_scope_device, ...); // invalid; value is only known at runtime
make_static_set<Key, cuda::thread_scope_device>(...); // valid; value is known at compile timeSince all other tparams can be deduced from the parameter pack of the factory, I had to move the non-deducible tparams to the front.
| struct extent : private detail::extent_base<SizeType> { | ||
| using value_type = typename detail::extent_base<SizeType>::value_type; ///< Extent value type |
There was a problem hiding this comment.
Adding this base class was also necessary so I could provide a trait which checks if a given type is cuco::extent-like
| template <typename Key> | ||
| struct key_equal_traits { | ||
| template <typename T, typename = std::void_t<>> | ||
| struct is_equal_functor : std::false_type { | ||
| }; | ||
|
|
||
| template <typename T> | ||
| struct is_equal_functor< | ||
| T, | ||
| std::void_t<std::enable_if_t<std::is_invocable_r_v<bool, T, Key const&, Key const&>>>> | ||
| : std::true_type { | ||
| }; | ||
|
|
||
| template <typename T> | ||
| using is_equal_functor_t = typename is_equal_functor<T>::type; | ||
|
|
||
| template <typename T> | ||
| static constexpr bool is_equal_functor_v = is_equal_functor<T>::value; | ||
| }; |
There was a problem hiding this comment.
This was a bit tricky. I needed a trait, which checks if a given user type T is a binary functor (Key, Key) -> bool. However, the find_arg function only knows of T, and does not directly know of Key. That's why this is a nested trait which can be passed as find_arg<key_equal_traits<Key>::template is_equal_functor_t>(...).
It works, but I'm not proud of it. So I'm very open to suggestions to make this more elegant to use.
| template <class Key, cuda::thread_scope Scope, class... Args> | ||
| constexpr auto make_static_set(Args&&... args) | ||
| { | ||
| // TODO don't repeat defaults |
There was a problem hiding this comment.
Yeah, that's also not so elegant. Currently, we need to mirror every default tparam from static_set explicitly. If we change the default in static_set, but forget to change it in the factory, that would be bad.
Maybe I can instantiate a set with default tparams first, and then extract the tparams from its member type aliases:
using default_set = static_set<Key>;
using default_allocator = typename default_set::allocator_type;
...
|
Why is it a factory instead of just a constructor? Factory functions are largely defunct as of C++17 and the introduction of CTAD. |
I fiddled around with this and the problem with the initial design was that it required a deduction guide in order to correctly deduce the tparams from the That said, I'm working on a fix that enables us to deduce all of the tparams from the Another point to consider is if we directly integrate this into the ctor, we always pay for the extra compile time introduced by the |
This PR adds a factory
make_static_set<Key, Scope>(args…), which takes a parameter pack of ctor arguments in any order and returns astatic_setobject.Closes #207