Skip to content
This repository was archived by the owner on Sep 18, 2024. It is now read-only.
This repository was archived by the owner on Sep 18, 2024. It is now read-only.

Summary of ABQ comments on span #46

Open
@dhollman

Description

@dhollman

I'm going through the comments on span from Albuquerque to both understand where span stands relative to incorporation into the IS (and as it relates to P0546, P0856, and P0860) and to make sure we haven't made the same mistakes in mdspan. I'll enumerate summaries of the issues here (some with bulleted responses about how we've treated that in mdspan)

Comments from a thread titled "Questions raised while trying to implement " on the lib-ext reflector:

  • The comparison operators should work with differing extents, particularly if the extent of one of them is dynamic
    • mdspan doesn't define comparison operators, not even operator== (could this be a problem/inconsistency for some people? Probably it's okay to just tell people to convert to span first, especially with the potential for deduction guides on the conversion operators)
  • The comparison operators should be able to compare spans of unequal extents, since std::equal and std::lexigraphical_compare do this
  • span<int> and span<long> should be comparable
  • The comparison operators take their parameters by const reference rather than by value, even though the paper itself recommends passing span by value
    • We should change the free function subspan to take an mdspan by value for the same reason
  • Contiguous iterators and contiguous containers: span currently has (or had?) a constructor that takes a ContiguousContainer (i.e., a Container whose iterators meet the requirements of contiguous iterators [27.2.1]). This isn't implementable via SFINAE (or any other compile-time technique) since contiguous iterator is a runtime property.
    • mdspan doesn't have this constructor. It doesn't look like we're using the term "contiguous" in any contexts that would imply the need for SFINAE, but we should double-check this
  • If the size of the span (in bytes) is larger than numeric_limits<ptrdiff_t>::max() (e.g., as could be the case if sizeof(value_type) is much larger than 1), then as_bytes() and as_writable_bytes() could introduce undefined behavior.
    • This applies to us also. We should consider using size_t here instead. (Also, seems like a good motivation for the T[][][] syntax, so that we don't have to use -1 for a magic extent value)
    • A later note says that LEWG has already discussed this and decided to stick with ptrdiff_t, so maybe we shouldn't change anything.
  • No reason for the nullptr_t constructor, since it's the same as the default constructor
    • Not sure I agree with this one. Implicit convertibility from a nullptr_t is a reasonable trait to have. Either way, we omitted discussion/description of this constructor in the paper, even though it's in the wording.

Other Pre-meeting comments

  • span<int, 42>.subspan<0>() shouldn't return a span<dynamic_extent>
    • This is another artifact of the dynamic_extent magic number. It doesn't look like we have that problem because our subspan extents are always given as normal function arguments rather than template arguments (though this seems like a bit of a shortcoming since it means we can't have a subspan with static extents other than the entirety of the parent's extent for a given rank)
  • all iterator functions shoudl be constexpr
    • mdspan is not iterable (?!?) so this doesn't apply.
  • Shouldn't have both length and size

Small Group discussion comments

  • Lots of discussion about constructors taking shared_ptr/unique_ptr (presumably the array partial specializations)
  • Something indecipherable about propagating noexcept (presumably relating to comparison operators?)
  • The static version should work with std::get, since it is like std::array (though then it would have to participate in structured binding?)
    • Probably not that applicable to mdspan. If you really need this, just convert to a span

Full group discussion

  • Maybe considering switching back to array_view, now that the term view does not imply immutable (presumably with some change to string_view?)
  • Poll to remove unique_ptr and shared_ptr constructors: 6-8-0-0-0
  • Poll to remove nullptr_t constructor: 4-5-3-0-0
  • Poll to remove span(pointer, pointer) constructor: 0-0-2-10-0

Other notes (not from span discussion, but encountered while working on it):

  • On the conversion constructors and assignment operators:
template< typename UType , typename ... UProperties >
constexpr mdspan( mdspan< UType , UProperties ... > const & ) noexcept;
template< typename UType , typename ... UProperties >
mdspan & operator = ( mdspan< UType , UProperties ... > const & ) noexcept;

One of the requirements is that "V::static_extent(r) == U::static_extent(r) or V::static_extent(r) == std::dynamic_extent for 0 < r < V::rank()". This should probably also include the possibility that U::static_extent(r) == std::dynamic_extent.

  • mdspan defines the constraints here in terms of std::is_assignable<U::pointer, V::pointer>, but span defines it in terms of accessing a U::value_type through a V::pointer meeting the rules for well-defined object access defined in [basic.lval]/8. Are these definitions exactly equivalent?

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions