Undefined behavior on partially filled span overflow

[justinboneh]

In case of overflow:

int arr[] = { 0, 1 , 2 };
ring_span<int> rs( arr, arr + dim(arr), arr + 1, 3 );

https://github.com/martinmoene/ring-span-lite/blob/dfb7c345d6ad856246df162bc891900a74963747/include/nonstd/ring_span.hpp#L432-L445 Should assert(first + size <= end); be added?

[Quuxplusone]

@justinboneh: Can you post a short complete example program that demonstrates the undefined behavior? In the snippet you posted, I don't see any UB.

Here's an example of a short complete program, but this one doesn't have any UB:

// https://godbolt.org/z/8wpnln
#include "ring_span.hpp"
#include <iostream>

int main() {
    int arr[] = { 0, 1 , 2 };
    nonstd::ring_span<int> rs( arr, arr + 3, arr + 1, 3 );
    std::cout << rs.front() << "\n";  // 1
    std::cout << rs.back() << "\n";  // 0
}

[justinboneh]

Sure, sorry for not being clear. You are right regarding the above example - it is actually correct. But consider the following case:

// https://godbolt.org/z/7juTdZ
#include <iostream>
#include <array>

using nonstd::ring_span;

int main() {
    std::array<int, 5> A = { 1,2,3,4,5 };
    ring_span<int> rs1(A.begin(), A.end(), A.begin(), 10); // <--- this will cause capacity to be smaller than size
    ring_span<int> rs2(rs1.begin(), rs1.end(), rs1.begin(), 9);  // <--- this will put the rs1's size as rs2's capacity, which will allow access violation
    std::cout << rs1.size() << "/" << rs1.capacity() << "\n";  // 10/5
    std::cout << rs2.size() << "/" << rs2.capacity() << "\n";  // 9/10
    std::cout << rs2.front() << "\n";  // 1
    std::cout << rs2.back() << "\n";  // ???
}

I think in general it's better not to allow a case where the size is bigger than the capacity (full() is unclear here too). A possible solution can be:

template< class ContiguousIterator >
ring_span(
    ContiguousIterator   begin
    , ContiguousIterator end
    , ContiguousIterator first
    , size_type          size
    , Popper popper = Popper()
) nsrs_noexcept
: m_data     ( &* begin )
, m_size     ( size     )
, m_capacity ( static_cast<size_type>( end   - begin ) )
, m_front_idx( static_cast<size_type>( first - begin ) )
, m_popper   ( std11::move( popper ) )
{
    assert(m_size <= m_capacity); // <--- invalid input
}

[Quuxplusone]

Ah, that makes more sense. Yes, it is user error to construct a ring_span and pass an initial size which is greater than the ring's actual capacity. That is, this constructor should have a precondition that size <= end - begin, and violating that precondition should be UB. As to whether that UB ought to be expressed as "we don't bother to check" (the status quo) or "we assert-fail" (your preference) or "even in release mode, we check and throw std::logic_error" or "we clamp the size to the capacity" or something else... I'll bow out here and leave the followup to Martin. :)

[martinmoene]

For now I'd suggest:

• to not introduce an exception that is not (yet) described in the proposal
• to not introduce behaviour that is not (yet) described in the proposal through clamping (although I somehow like that function :)
• add the suggested assert (debug mode)

would that be helpful enough?