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

Span utility #404

Merged
merged 5 commits into from
Jul 7, 2020
Merged

Span utility #404

merged 5 commits into from
Jul 7, 2020

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Jul 6, 2020

Implements some parts of std::span<> from C++20.

Taken from #359 and extended for needs in #403.

@codecov
Copy link

codecov bot commented Jul 6, 2020

Codecov Report

Merging #404 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master     #404    +/-   ##
========================================
  Coverage   99.16%   99.17%            
========================================
  Files          43       45     +2     
  Lines       13043    13165   +122     
========================================
+ Hits        12934    13056   +122     
  Misses        109      109            

@chfast chfast force-pushed the span branch 3 times, most recently from ce0ed76 to 97f73ad Compare July 6, 2020 09:31
@chfast chfast marked this pull request as ready for review July 6, 2020 09:31
@chfast chfast requested review from axic and gumb0 July 6, 2020 09:38
constexpr span() noexcept = default;
constexpr span(const span&) = default;

constexpr span(const T* begin, std::size_t size) noexcept : m_begin{begin}, m_size{size} {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if T is already const, is const needed here or how is it allowed at all?

Suggested change
constexpr span(const T* begin, std::size_t size) noexcept : m_begin{begin}, m_size{size} {}
constexpr span(T* begin, std::size_t size) noexcept : m_begin{begin}, m_size{size} {}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I mean it looks at least inconsistent with the m_begin definition above, it should be either both T* or both const T*.
But not sure if it would somehow break anything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, fixed.

/// https://en.cppreference.com/w/cpp/container/span
/// Only `const T` is supported.
template <typename T, typename = typename std::enable_if_t<std::is_const_v<T>>>
class span
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to fall back to native const if compiling on C++20 or it is not worth the hassle (it is not any faster/better than this below)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the question.

This is compatible with C++20 and you can just start using std::span<const uint64_t>. The C++20 also supports the std::span<uint64_t>.

I disabled the usage of the later, because we don't need it. And also because of the implicit conversions involved. E.g. conversion from const std::vector<uint64_t>& to span<uint64_t> should be disallowed. I prefer not to handle this in the implementation as long as we don't need it.

If by native const you mean const span<uint64_t> it is not useful as an argument type (similar to const int argument type).
I think about span as a reference type similar to pointers:

  • span<int> is as int*,
  • span<const int> is as const int*,
  • const span<int> is as int* const,
  • const span<const int> is as const int* const.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is compatible with C++20 and you can just start using std::span. The C++20 also supports the std::span<uint64_t>.

The question was can this header be done in a way such that any code using it will either use the native C++20 implementation or this fallback, depending on compiler settings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this can be done. I can do it here after #405.

@chfast chfast force-pushed the span branch 2 times, most recently from b6e48f1 to d0fb9cc Compare July 6, 2020 15:16
lib/fizzy/span.hpp Outdated Show resolved Hide resolved
Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming some things will be squashed.

@chfast chfast merged commit 9a6fc2e into master Jul 7, 2020
@chfast chfast deleted the span branch July 7, 2020 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants