kokkos / mdspan

Reference implementation of mdspan targeting C++23
Other
418 stars 69 forks source link

Bugs to fix for P0009R13 #69

Closed mhoemmen closed 3 years ago

mhoemmen commented 3 years ago

@crtrott I compared the current P0009 draft and the mdspan implementation, and found the following inconsistencies and issues.

default_accessor:

  Spec:
    constexpr typename offset_policy::pointer
        offset(pointer p, size_t i) const noexcept;

  impl:
    constexpr pointer
    offset(pointer p, size_t i) const noexcept {
      return p + i;
    }

extents:

  In implementation, not in spec:

  MDSPAN_INLINE_FUNCTION_DEFAULTED constexpr extents(extents&&) noexcept = default;
  MDSPAN_INLINE_FUNCTION_DEFAULTED _MDSPAN_CONSTEXPR_14_DEFAULTED extents& operator=(extents&&) noexcept = default;
  MDSPAN_INLINE_FUNCTION_DEFAULTED ~extents() noexcept = default;

  // in spec; impl claims this should be explicit
  template<class SizeType>
    constexpr extents(const array<SizeType, rank_dynamic()>&) noexcept;

  // impl has operator!=, but spec does not;
  // should be unnecessary due to synthesis of operator!= from operator==

  // impl has CTAD for extents; spec does not
  #if defined(_MDSPAN_USE_CLASS_TEMPLATE_ARGUMENT_DEDUCTION)
  template <class... SizeTypes>
  extents(SizeTypes...)
    -> extents<detail::__make_dynamic_extent<SizeTypes>()...>;
  #endif

layout_*:

  Spec mapping: missing move constructor and move assignment operator
  Do we want to add move version of OtherExtents constructor?  Not in impl.

  Impl has operator== and operator!=; spec only has operator==

mdspan:

  Spec: using element_type = typename accessor_type::element_type;
  Impl: using element_type = ElementType;

  // noexcept in impl, not in spec
  template<class... SizeTypes>
  explicit constexpr mdspan(pointer p, SizeTypes... dynamic_extents);

  // noexcept in impl, not in spec
  template<class SizeType, size_t N>
  explicit constexpr mdspan(pointer p, const array<SizeType, N>& dynamic_extents);

  // TODO @proposal-bug This is missing from the proposal.
  explicit constexpr mdspan(pointer p, const extents_type& exts)
    noexcept
    : __members(p, __map_acc_pair_t(mapping_type(exts), accessor_type()))
  { }

  // Do the following need noexcept specifications in the spec?
  constexpr mdspan(pointer p, const mapping_type& m);
  constexpr mdspan(pointer p, const mapping_type& m, const accessor_type& a);
  template<class OtherElementType, class OtherExtents, class OtherLayoutPolicy, class OtherAccessorPolicy>
    constexpr mdspan(
      const mdspan<OtherElementType, OtherExtents, OtherLayoutPolicy, OtherAccessorPolicy>& other);

  // TODO @proposal-bug The proposal is missing constexpr here
  MDSPAN_INLINE_FUNCTION constexpr
  accessor_type accessor() const { return __accessor_ref(); };

  // Spec still has this!  Should we remove it?
  constexpr span<element_type> span() const noexcept;

  // Consistent in both spec and impl,
  // but should this be noexcept in spec?
  // Does the layout mapping not promise that stride is noexcept?
  constexpr size_type stride(size_t r) const { return map_.stride(r); }

mdspan deduction guides:

  // Spec writes layout_type; impl writes layout.  The "TODO" in the implementation's comment is done.

  template <class ElementType, class MappingType>
  mdspan(ElementType*, const MappingType&)
    -> mdspan<ElementType, typename MappingType::extents_type, typename MappingType::layout>;

  template <class ElementType, class MappingType, class AccessorType>
  mdspan(ElementType*, const MappingType&, const AccessorType&)
    -> mdspan<ElementType, typename MappingType::extents_type, typename MappingType::layout, AccessorType>;
mhoemmen commented 3 years ago

PR https://github.com/ORNL/cpp-proposals-pub/pull/153 addresses all the major concerns above.