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

Adding missing span deduction guides #891

Merged

Conversation

JordanMaples
Copy link
Contributor

I noticed while experimenting with std::span that some of the functionality wasn't 1:1 regarding CTAD.
The deduction of container types was missing. The following std::span example compiles without any problems, while the the gsl equivalent does not.

Ex: Current behavior

std::vector v{1,2,3,4}; 
std::span sp{v}; // no problem, compiles
std::vector v{1,2,3,4}; 
gsl::span sp{v}; // fails to compile. gsl::span<int>, however does compile

By examining the container's value_type in the deduction guide, we're able to deduce the internal type and construct the span. This more closely aligns gsl::span's behavior to std::span.

include/gsl/span Outdated
Comment on lines 743 to 747
template <class Container>
span(Container&)->span<typename Container::value_type>;

template <class Container>
span(const Container&)->span<const typename Container::value_type>;
Copy link
Member

Choose a reason for hiding this comment

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

If you only care about containers, this is fine. Non-containers sometimes have const-qualified elements even when they are themselves not const, which will break this. For example, in:

#include <gsl/span>
#include <string_view>
#include <type_traits>

using namespace std::literals;

int main() {
    auto s = "Hello, World!"sv;
    gsl::span x{s}; // Line 9
    static_assert(std::is_same_v<decltype(x)::element_type, const char>);
}

CTAD deduces span<char> on line 9, resulting in the compiler complaining that initialization from const char* is ill-formed.

I suggest instead:

Suggested change
template <class Container>
span(Container&)->span<typename Container::value_type>;
template <class Container>
span(const Container&)->span<const typename Container::value_type>;
template <class Container,
class Element = std::remove_pointer_t<decltype(std::declval<Container&>().data())>>
span(Container&)->span<Element>;
template <class Container,
class Element = std::remove_pointer_t<decltype(std::declval<const Container&>().data())>>
span(const Container&)->span<Element>;

Copy link
Member

@CaseyCarter CaseyCarter May 29, 2020

Choose a reason for hiding this comment

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

Okay, my OCD made me test these, so I'll submit a PR (JordanMaples#6) with this change and the updated test.

@JordanMaples JordanMaples merged commit 794d7bb into microsoft:master May 29, 2020
@JordanMaples JordanMaples deleted the dev/jomaples/missing_span_ctad 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.

2 participants