kokkos / mdarray

Other
10 stars 9 forks source link

Implemented uses-allocator constructors #12

Closed AnabelSMRuggiero closed 2 years ago

AnabelSMRuggiero commented 2 years ago

Still need to make a final clean up pass (tests, clean up the dot_example, confirm the implementation works with C++17/14/11) but implemented constructors for uses-allocator construction and an example showing a std::pmr::vector passing it's allocator down to mdarrays it holds.

As for the use of raw SFINAE: I attempted to use requirements instead, but I'm pretty sure SFINAE is needed to prevent bifrucating the implementation into a partial specialization. The constructor needs to get the allocator type in a dependent context, and that can only be done during template argument deduction without causing a hard compile error in cases where the allocator_type member doesn't exist. Concepts and requirements are checked during overload resolution, after template argument deduction.

Of course, I could have missed something that allows a SFINAEless version to work in C++20.

crtrott commented 2 years ago

Hey thanks for this PR, as I said in the issue I am rewriting the mdarray design a bit right now, getting rid of the ContainerPolicy, and I am looking into which constructors we actually should have. With regard to the allocator ones, I feel torn about the constructor with an integer pack. Everywhere else in Containers, allocator is the last argument. That obviously doesn't work with index packs. Right now I am trending towards leaving those out, i.e. a user would need to provide an extents object plus an allocator.

Thoughts?

AnabelSMRuggiero commented 2 years ago

Mark had mentioned to me that you were starting to get a bunch of work done redesigning stuff, which was the impetus for me submitting the PR a bit early. Removing the container policy shouldn't change how I implemented the constructors with allocators much nor should removing a constructor; the hard part was figuring out a general plan for them. Basically boils down to the right combination of custom type traits, and reimplementing all of the constructors with a new argument. I actually think removing the container policy is a great place to end up at as one of the questions I had was if the container_policy base could be exposed so users can cut out a bunch of boilerplate. On the integer pack constructor, I really like the idea of the constructors lining up with mdspan's, but I had failed to get it to work here. If I could just say container_type::allocator_type, it'd work as a default parameter, but I did have just have a bit of a crazy idea that may work... which will have to wait til tomorrow for me to try.

AnabelSMRuggiero commented 2 years ago

Turns out, my insane idea ended up working. Got trailing allocator convention to work with an index pack.

AnabelSMRuggiero commented 2 years ago

If you'd like, I can update the implementation of these constructors to match as a new PR.