A few small suggestions

[chris0e3]

Great job. Thanks for creating & releasing this. Have you considered offering it for use in libc++/clang? It would be good if you could add the static size optimisation (at some point).

I have been experimenting with it & testing it with Clang 6.0 & C++17.

Here’s my feedback & suggestions:

• Add #include <algorithm> (for lexicographical_compare).
• The with_container_t stuff appears redundant (with Clang 6.0 & C++17).
• In namespace nonstd add using span_lite::as_bytes; and using span_lite::as_writeable_bytes; (after line 932).
• The operator== fails to compile with:

  static const uint8_t data[] = { 0,1,2,3,4,5,6,7,8,9,10 };
  span<uint8_t const> span_data(data);
  assert(make_span(data) == span_data);   // Compile error

  I fixed it by adding template<class T> struct is_span_oracle<span<T>> : std::true_type {}; (by the current is_span_oracle) and changed operator==, != etc. to something like:

  
  template<class T1, index_t L1, class T2, index_t L2> inline constexpr bool
  operator==(span<T1, L1> l, span<T2, L2> r)
  { return (l.cbegin() == r.cbegin() && l.size() == r.size()) ||
           equal(l.cbegin(), l.cend(), r.cbegin(), r.cend()); }

template<class T1, index_t L1, class T2, index_t L2> inline constexpr bool operator<(span<T1, L1> l, span<T2, L2> r) { return lexicographical_compare(l.cbegin(), l.cend(), r.cbegin(), r.cend()); }

template<class T1, index_t L1, class T2, index_t L2> inline constexpr bool operator!=(span<T1, L1> l, span<T2, L2> r) { return !(l == r); }

template<class T1, index_t L1, class T2, index_t L2> inline constexpr bool operator<=(span<T1, L1> l, span<T2, L2> r) { return !(r < l); }

template<class T1, index_t L1, class T2, index_t L2> inline constexpr bool operator>(span<T1, L1> l, span<T2, L2> r) { return (r < l); }

template<class T1, index_t L1, class T2, index_t L2> inline constexpr bool operator>=(span<T1, L1> l, span<T2, L2> r) { return !(l < r); }

* The `as_bytes` & `as_writeable_bytes` have a mistake in their disabled lines (closing paren in wrong plac)e:
    `return { reinterpret_cast< std::byte const * >( spn.data(), spn.size_bytes() ) };`
should be:
    `return { reinterpret_cast<std::byte const*>(spn.data()), spn.size_bytes() };`
    `                                                      ^`
And
    `return { reinterpret_cast< std::byte * >( spn.data(), spn.size_bytes() ) };`
should be:
    `return { reinterpret_cast<std::byte*>(spn.data()), spn.size_bytes() };`
    `                                                ^`
* I added the following helpers along with the `make_span` functions:

template inline constexpr auto byte_span(T& t) noexcept { return span<byte, sizeof(t)>(reinterpret_cast<byte*>(&t), sizeof(t)); }

template inline constexpr auto byte_span(T const& t) noexcept { return span<byte const, sizeof(t)>(reinterpret_cast<byte const*>(&t), sizeof(t)); }

* I also added `cdata`, `front` & `back` member functions (with the obvious impls).
* Additionally I added the following (as C++ has no operator===):

template<class Span2 = span<T, Extent>>
constexpr bool same(Span2 s) const noexcept
{
    return static_cast<void const*>(data()) == s.data() && size() == s.size();
}

[martinmoene]

Thanks for your detailed feedback and suggestions!

The underlying idea of this implementation of std::span (or "std::span" if you like) is to provide a path to use span with C++98 and later and follow std::spans specification insofar possible under C++98, C++11, C++14. This leads to non-standard workarounds such as make_span().

To prevent surprises when transitioning to std::span I'd mark back(), front() and cdata() that you suggest as extensions that must be explicitly enabled. To note: cdata() seems unique.

The with_container_t stuff appears redundant (with Clang 6.0 & C++17).

Providing span(with_container_t, ...) and –for symmetry– make_span(with_container_t, ...) even with C++17 [1], enables to upgrade code using it and developed under pre-C++17 to be first compiled under C++17 and than to be transformed.

[1] If requested via -Dspan_CONFIG_PROVIDE_MAKE_SPAN=1.

[martinmoene]

@chris0e3 In byte_span(), is there a reason you write sizeof(t) and not sizeof(T) in return span<byte, sizeof(t)>(...) ?

[chris0e3]

I agree that you should follow ‘the spec’. However, the spec has yet to be finalised - and as such there is the possibility to change it. I think that make_span, byte_span, front, back & same would be useful additions to the spec.

For some reason I had convinced myself that cdata had been standardised for vector, string, etc., (like cbegin & cend). But I can find no mention of it anywhere. Perhaps it’s not needed.

I now see how with_container_t may be needed with C++17. However, it’s non-std & not required by C++17 so it should be possible to exclude it if using C++17. This would also help in the …14…17…20 transition.

Anyway, after writing the original message it occurred to me that my same & operator== functions were incorrect on the occasion that the data_s & size_s were equal, but the value_types were not.

    template<class Span2 = span<T, Extent>>
    constexpr bool same(Span2 s) const noexcept
    {
        return is_same_v<value_type, typename Span2::value_type> &&
               static_cast<void const*>(data()) == s.data() && size() == s.size();
    }

and

template<class T1, index_t L1, class T2, index_t L2> inline constexpr bool
operator==(span<T1, L1> l, span<T2, L2> r)
            { return l.same(r) || equal(l.cbegin(), l.cend(), r.cbegin(), r.cend()); }

should fix the problems.

The following tests cover the changes:

    static uint8_t const data[] = { 0,1,2,3,4,5,6,7,8,9,10 };
    static float const farray[4] = { 0,1,2,3 };
    auto fspan1 = std::make_span(farray);
    assert(fspan1.data() == farray);
    assert(fspan1.size() == array_size(farray));
    auto fspan2 = std::byte_span(farray[0]);
    assert(static_cast<void const*>(fspan1.data()) == fspan2.data());
    assert(fspan1.size() == fspan2.size());
    assert(!fspan1.same(fspan2));

    auto bspan4 = std::make_span(data, 4);
    assert(bspan4 == fspan1);
    assert(fspan1 == bspan4);
    assert(!fspan1.same(bspan4));
    assert(as_bytes(fspan1) != as_bytes(bspan4));
    assert(!as_bytes(fspan1).same(as_bytes(bspan4)));

    union { int i; float f; char c; } u{0x12345678};
    auto uspan1 = std::make_span(&u.i, 1);
    auto uspan2 = std::make_span(&u.f, 1);
    auto uspan3 = std::make_span(&u.c, 1);
    assert(static_cast<void const*>(uspan1.data()) == uspan2.data());
    assert(uspan1.size() == uspan2.size());
    assert(static_cast<void const*>(uspan1.data()) == uspan3.data());
    assert(uspan1.size() == uspan3.size());
    assert(!uspan1.same(uspan2));
    assert(!uspan1.same(uspan3));
    assert(!uspan2.same(uspan3));
    assert(uspan1 != uspan2);
    assert(uspan1 != uspan3);
    assert(uspan2 != uspan3);
    assert(as_bytes(uspan1).same(as_bytes(uspan2)));
    assert(!as_bytes(uspan1).same(as_bytes(uspan3)));
    assert(!as_bytes(uspan2).same(as_bytes(uspan3)));

[chris0e3]

@chris0e3 In byte_span(), is there a reason you write sizeof(t) and not sizeof(T) in return span<byte, sizeof(t)>(...) ?

@martinmoene, I sometimes prefer that form (in non-template code) as it’s possible to change the type of a var in a declaration and forget to change other sizeof(TYPE) code. That can come back & bite (byte) you much later. I think it makes no difference in this case.

Feel free to use, not use, or change any code I have submitted - as you see fit.

[martinmoene]

@chris0e3

Re: sizeof(t) vs sizeof(T): memory refreshed ;)

Have you considered offering it for use in libc++/clang?

It is my intention that that works. I'll have to glean the Travis knowledge to also have it verified, e.g. at Catch.

I think that make_span, byte_span, front, back & same would be useful additions to the spec.

Apart from make_span, I agree, with the caveat that they are opt-in extensions while they are not (yet) part of the specification. make_span is a workaround for pre-C++17 environments.

[with_container]: However, it’s non-std & not required by C++17 so it should be possible to exclude it if using C++17.

I agree.

member same()

1. In your suggestion, same() is a member function, whereas the comparison functions such as operator==() are non-member functions. Is there a reason to deviate and have same() as a member?

2. Is same the 'right' name?

   • a.equals(b); equal(a, b);
   • a.strictly_equals(b); strictly_equal(a, b);
   • a.sames(b); a.is_same_as(b); same(a, b); is_same(a, b);
   • a.identity(b); identity(a, b);
   • ...

[martinmoene]

Plan:

• [x] Activate tests for make_span() in CMake configuration
• [x] Unconditionally include test taking spans of different cv-ness
• [x] Add #include <algorithm>
• [x] Fix missing using of as_bytes() and as_writeable_bytes() in namespace nonstd
• [x] Fix closing parenthesis in disabled line of as_bytes() and as_writeable_bytes()
• [x] Add failing test for span heterogeneous comparison
• [x] Fix span comparisons to be heterogeneous
• [x] Add non-standard byte_span() creator functions
• [x] Add members back() and front(), span_CONFIG_PROVIDE_BACK_FRONT
• [x] Add same(a,b), operator==() using same(), span_CONFIG_PROVIDE_SAME
• [x] Add span_CONFIG_PROVIDE_WITH_CONTAINER_TO_STD: 98, 03, 11, 14, 17, 20, ... (to x: including x)
• [x] Change: span_CONFIG_PROVIDE_MAKE_SPAN_TO_STD: 98, 03, 11, 14, 17, 20, ... (to x: including x)
• [x] Add span_CONFIG_PROVIDE_CONSTRUCTION_FROM_STDARRAY_ELEMENT_TYPE
• [x] Introduce can_construct_from<Container, ElementType>
• [x] Release span v0.2.0
• [x] On the ISO C++ Lib mailing list, ask to consider back(), front(), swap() and same() with the updated operator==() for the standardisation of span.

Postpone:

• [ ] Add span_CONFIG_PROVIDE_ALL_EXTENSIONS (excluding make_span() workaround)
• [ ] Add clang/libc++ to Travis configuration
• [ ] Add the static size optimisation (at some point).

[chris0e3]

@martinmoene

Have you considered offering it for use in libc++/clang? It is my intention that that works. I'll have to glean the Travis knowledge to also have it verified, e.g. at Catch.

I meant offering the source for inclusion (as part of) libc++.

Apart from make_span, I agree, with the caveat that they are opt-in extensions while they are not (yet) part of the specification. make_span is a workaround for pre-C++17 environments.

I didn’t realise make_span was completely redundant in C++17. I wonder which other std::make_… functions are also redundant, make_pair?

In your suggestion, same() is a member function, whereas the comparison functions such as operator==() are non-member functions. Is there a reason to deviate and have same() as a member?

I meant to mention that. It could be a free function. It’s good for same to be different from operator== so users don’t confuse them. Perhaps they’ll add an operator=== in C++29 …

Is same the 'right' name?

a.equals(b); equal(a, b); a.strictly_equals(b); strictly_equal(a, b); a.sames(b); a.is_same_as(b); same(a, b); is_same(a, b); a.identity(b); identity(a, b);

Not equal or equals as operator== already calls a std::equal which has well defined semantics. Not strictly_… as std::equal is ‘strictly equal’. Not is_same as is_… usually implies a type trait (to me) and the std is empty() not is_empty(). (These 2 are where I got my inspiration.) is_same_as & same_as don’t really work for free functions & the extra words are unnecessary for a member function. Not identity as that can be confused with matrix identity. But identical could work.

Note: basic_string_view has compare member functions while operator== etc. are free. (It also has front & back member functions.)

[martinmoene]

@chris0e3

Have you considered offering it for use in libc++/clang?

It is my intention that that works. I'll have to glean the Travis knowledge to also have it verified, e.g. at Catch.

I meant offering the source for inclusion (as part of) libc++

Ah, I think there's no need to 'alert' Marshall Clow @mclow, Jonathan Wakely @jwakely and @StephanTLavavej on this to craft span into liblibstdstlstlc++++ (oh, I just did ;)

If your remark concerns promoting back(), front(), same() with the updated operator==() for standardisation, perhaps a good path is to draw attention to these on the ISO C++ Lib mailing list.

If you like I can do that.

[chris0e3]

If your remark concerns promoting back(), front(), same() with the updated operator==() for standardisation, perhaps a good path is to draw attention to these on the ISO C++ Lib mailing list.

No, actually my suggestion stands regardless of any additions to the (proposed) standard. Clang/LLVM’s libc++ appears to lack <span>. I see none in http://llvm.org/svn/llvm-project/libcxx/trunk/include/. I thought you could help. (It’s your code. It’s entirely up to you.)

If you like I can do that.

Go for it!

Also, I notice that std::string_view http://en.cppreference.com/w/cpp/string/basic_string_view has a swap member function. As std::span has operator= perhaps it should also have a swap.

[mclow]

Just FYI - I have an implementation that I haven't yet merged into libc++. It's waiting on more complete tests. I'll probably land it when I get back from C++Now.

[martinmoene]

@mclow Thanks for letting us know Marshall.

[mclow]

@martinmoene, @chris0e3 - thanks for thinking of me.

[martinmoene]

Remaining -postponed- items reflected in issues #7, #8 and #9.