A few small suggestions #3

chris0e3 · 2018-05-01T07:23:48Z

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<class T> inline constexpr auto
byte_span(T& t) noexcept
    { return span<byte, sizeof(t)>(reinterpret_cast<byte*>(&t), sizeof(t)); }

template<class T> 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();
    }

The text was updated successfully, but these errors were encountered:

martinmoene · 2018-05-01T13:17:45Z

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.

For std::lexicographical_compare(), std::equal()

@chris0e3

…hanks to @chris0e3)

@chris0e3

…nks to @chris0e3) also in as_writeable_bytes()

span comparison are not up to date with /heterogeneous/ comparisons introduced in p0122r7.

…chris0e3

…chris0e3)

martinmoene · 2018-05-01T16:25:21Z

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

chris0e3 · 2018-05-01T19:13:30Z

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 · 2018-05-01T19:25:15Z

@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.

…chris0e3

…chris0e3)

martinmoene · 2018-05-01T21:40:54Z

@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()

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?
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 · 2018-05-01T21:55:03Z

chris0e3 · 2018-05-01T22:41:59Z

@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 · 2018-05-02T08:07:34Z

@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.

controlled via span_CONFIG_PROVIDE_BACK_FRONT

controlled via span_CONFIG_PROVIDE_SAME

chris0e3 · 2018-05-02T19:40:57Z

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 · 2018-05-02T19:58:08Z

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 · 2018-05-02T20:39:26Z

@mclow Thanks for letting us know Marshall.

mclow · 2018-05-02T22:29:20Z

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

martinmoene · 2018-05-04T07:42:04Z

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

martinmoene added a commit that referenced this issue May 1, 2018

Fix missing #include<algorithm> (issue #3, thanks to @chris0e3)

9c216c5

For std::lexicographical_compare(), std::equal()

martinmoene added a commit that referenced this issue May 1, 2018

Fix missing using of as_bytes() and as_writeable_bytes() (issue #3, t…

9fce5a8

…hanks to @chris0e3)

martinmoene added a commit that referenced this issue May 1, 2018

Fix closing parenthesis in disabled line of as_bytes() (issue #3, tha…

e3f509d

…nks to @chris0e3) also in as_writeable_bytes()

martinmoene added a commit that referenced this issue May 1, 2018

Add failing test for span comparison, issue #3 (thanks to @chris0e3)

eb54ed6

span comparison are not up to date with /heterogeneous/ comparisons introduced in p0122r7.

martinmoene added a commit that referenced this issue May 1, 2018

Fix span comparisons to be heterogeneous as per issue #3 (thanks to @…

536db90

…chris0e3)

martinmoene added a commit that referenced this issue May 1, 2018

Guard test for issue #3 with span_CONFIG_PROVIDE_MAKE_SPAN

58e0025

martinmoene added a commit that referenced this issue May 1, 2018

Add non-standard byte_span() creator functions (issue #3, thanks to @…

3b4605e

…chris0e3)

martinmoene added a commit that referenced this issue May 2, 2018

Add back() and front() (issue #3, thanks to @chris0e3)

477e3e7

controlled via span_CONFIG_PROVIDE_BACK_FRONT

martinmoene added a commit that referenced this issue May 2, 2018

Add function same() (issue #3, thanks to @chris0e3)

cea350a

controlled via span_CONFIG_PROVIDE_SAME

This was referenced May 4, 2018

Add clang/libc++ to Travis configuration #7

Open

Specialize span for dynamic extent (optimize static size) #8

Open

[pm] Add span_CONFIG_PROVIDE_ALL_EXTENSIONS (excluding make_span() workaround) #9

Open

martinmoene closed this as completed May 4, 2018

martinmoene mentioned this issue May 18, 2018

gsl::span front() and back() methods are missing microsoft/GSL#684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A few small suggestions #3

A few small suggestions #3

chris0e3 commented May 1, 2018 •

edited

Loading

martinmoene commented May 1, 2018 •

edited

Loading

martinmoene commented May 1, 2018

chris0e3 commented May 1, 2018 •

edited

Loading

chris0e3 commented May 1, 2018

martinmoene commented May 1, 2018

martinmoene commented May 1, 2018 •

edited

Loading

chris0e3 commented May 1, 2018

martinmoene commented May 2, 2018 •

edited

Loading

chris0e3 commented May 2, 2018

mclow commented May 2, 2018

martinmoene commented May 2, 2018

mclow commented May 2, 2018

martinmoene commented May 4, 2018

A few small suggestions #3

A few small suggestions #3

Comments

chris0e3 commented May 1, 2018 • edited Loading

martinmoene commented May 1, 2018 • edited Loading

martinmoene commented May 1, 2018

chris0e3 commented May 1, 2018 • edited Loading

chris0e3 commented May 1, 2018

martinmoene commented May 1, 2018

martinmoene commented May 1, 2018 • edited Loading

chris0e3 commented May 1, 2018

martinmoene commented May 2, 2018 • edited Loading

chris0e3 commented May 2, 2018

mclow commented May 2, 2018

martinmoene commented May 2, 2018

mclow commented May 2, 2018

martinmoene commented May 4, 2018

chris0e3 commented May 1, 2018 •

edited

Loading

martinmoene commented May 1, 2018 •

edited

Loading

chris0e3 commented May 1, 2018 •

edited

Loading

martinmoene commented May 1, 2018 •

edited

Loading

martinmoene commented May 2, 2018 •

edited

Loading