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

gsl::span P1976R2 implemenation #887

Merged
merged 9 commits into from
May 21, 2020

Conversation

JordanMaples
Copy link
Contributor

@JordanMaples
Copy link
Contributor Author

I'll keep this in draft form until I get clean builds of Appveyor and Travis, I'm guessing there will be some issues that pop up with some of the older compilers.

include/gsl/span Outdated Show resolved Hide resolved
include/gsl/span Outdated Show resolved Hide resolved
include/gsl/span Outdated Show resolved Hide resolved
tests/span_ext_tests.cpp Outdated Show resolved Hide resolved
tests/span_tests.cpp Outdated Show resolved Hide resolved
include/gsl/span Outdated Show resolved Hide resolved
include/gsl/string_span Outdated Show resolved Hide resolved
tests/string_span_tests.cpp Outdated Show resolved Hide resolved
@JordanMaples JordanMaples marked this pull request as ready for review May 20, 2020 21:47
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Looks good - just a formatting inconsistency I noticed.

include/gsl/span Outdated Show resolved Hide resolved
Comment on lines +374 to +379
template <std::size_t Ext>
constexpr extent_type<Ext>::extent_type(extent_type<dynamic_extent> ext)
{
Expects(ext.size() == Ext);
}

Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure whether this actually needs to be an out-of-line definition. My understanding is that member function bodies aren't instantiated until needed (this is what lets list<T>::sort() require op< from T only if sort() is actually called). So you should be able to define this within the class definition, without fear of prematurely instantiating the primary template for extent_type<dynamic_extent> before the explicit specialization has been seen. But if you're especially concerned about that, I think I would recommend declaring the primary template and then declaring the explicit specialization, so you can definitely define this member function within the class definition.

include/gsl/span Outdated
@@ -420,18 +420,28 @@ public:
constexpr span() noexcept : storage_(nullptr, details::extent_type<0>())
{}

constexpr span(pointer ptr, size_type count) noexcept : storage_(ptr, count)
template<std::size_t SpanExtent = Extent, std::enable_if_t<SpanExtent != gsl::dynamic_extent, int> = 0>
Copy link
Member

Choose a reason for hiding this comment

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

Within the gsl namespace, you should never need to qualify gsl::dynamic_extent - you can simply say dynamic_extent without fear of ambiguity with std::dynamic_extent (even if the user has using namespace std;). This is because unqualified name lookup will start within gsl and find the declaration there - it won't reach into the global namespace and won't consider any using-directives/using-declarations there either. The same is true for all non-functions in gsl (e.g. types, alias templates, variable templates, etc.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like there's a mix of dynamic_extent and gsl::dynamic_extent all throughout this file. I'll file an issue and separate PR to make it more cohesive.

include/gsl/span Show resolved Hide resolved
include/gsl/span Outdated Show resolved Hide resolved
include/gsl/span Outdated Show resolved Hide resolved
include/gsl/span Outdated Show resolved Hide resolved
include/gsl/span Outdated Show resolved Hide resolved
tests/span_tests.cpp Outdated Show resolved Hide resolved
@JordanMaples
Copy link
Contributor Author

Thanks again @CaseyCarter and @StephanTLavavej for the code review!

@JordanMaples JordanMaples merged commit 8cd1941 into microsoft:master May 21, 2020
@JordanMaples JordanMaples deleted the dev/jomaples/p1976r2 branch August 10, 2020 23:47
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