eyalroz / cuda-api-wrappers

Thin, unified, C++-flavored wrappers for the CUDA APIs
BSD 3-Clause "New" or "Revised" License
799 stars 80 forks source link

unique_span deleted copy constructors should ignore source deleter type #662

Closed kdkavanagh closed 3 months ago

kdkavanagh commented 4 months ago

I initially didnt catch that cuda::memory::host::make_unique_span and cuda::memory::device::make_unique_span do not both return the same type due to the different deleter required for the device span. I missed it because of the identically-named but differently-defined using unique_span = ... aliases for device/host.

My issue arose when I assigned the results of both those calls to an object of type cuda::unique_span<T>.
Even though copy-constructors are explicitly deleted, unique_span has a ctor which accepts a std::span as a copy:

explicit unique_span(span_type span) noexcept : span_type{span} { }

Since a cuda::memory::device::unique_span is not technically the same type as cuda::unique_span, the deleted copy constructor is ignored, and since the unique_span types extend std::span, that unique_span(span_type span) constructor is called, ending up with two unique_spans "owning" the data

A fix for this would probably add an explicit deleted constructor for any unique_span type rather than just a deleted ctor for the same unique_span type:

template<typename OtherT, typename OtherDeleter>
explicit unique_span(const unique_span<OtherT, OtherDeleter>&) = delete;
eyalroz commented 4 months ago

A few thoughts...

kdkavanagh commented 4 months ago

Move constructors definitely needed + are perfectly appropriate (i.e same as std::unique_ptr). Without them you pretty much wont be able to store the type in any container class

re: explicit, just a bad copy-paste into github. Definitely not needed