eyalroz / cuda-api-wrappers

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

Address unique_span issues from cr.SX #665

Open eyalroz opened 1 month ago

eyalroz commented 1 month ago

I've asked for a code review of my unique_span class, on codereviews.stackexchange.com, and got one.

Issues brought up:

  1. Can simplify swap()
  2. No need to delete copy ctors other than those C++ might itself generate
  3. Assignment to the base-class is long-winded
  4. We should be able to convert from a span of T to a span of const T.
  5. We don't know that release() is noexcept... so making any of our [move] constructors noexcept is a leap of faith.
  6. Deleter is not stored
  7. Factory method is not allocator-aware
  8. Factory method requires default-constructible element type
  9. No Tests

Well, my decision about all of those:

  1. Do it.
  2. It's a form of documentation-via-code... plus I'm not 100% sure this can't happen, e.g. via type conversion which isr's b
  3. Neat, let's do it. But - assigning {} is a bit confusing for the reader, IMHO, so let's keep that part explicit.
  4. Do it.
  5. Do it - drop the noexcept.
  6. Mulling over whether I want to support a stateful deleter. For now - not doing it.
    edit: see #678 .
  7. Not right now.
  8. Not right now.
  9. Ah, tests... we all want tests.
eyalroz commented 3 weeks ago

Some issues can't be fully addressed due to the use of the "poor man's span" with earlier C++ versions, as the base class.