Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address unique_span issues from cr.SX #665

Open
eyalroz opened this issue Aug 13, 2024 · 1 comment
Open

Address unique_span issues from cr.SX #665

eyalroz opened this issue Aug 13, 2024 · 1 comment

Comments

@eyalroz
Copy link
Owner

eyalroz commented Aug 13, 2024

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 Change unique_span to take a deleter object taking a span, not a template param accepting taking pointer #678 .
  7. Not right now.
  8. Not right now.
  9. Ah, tests... we all want tests.
@eyalroz
Copy link
Owner Author

eyalroz commented Aug 21, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant