Some notes on the proposal

[niliopoulos]

Hello, This is a review of the current HEAD (6abdf1d8d) of the P0009 proposal text.

General

1. The following can be said for the gsl::span as well but I think some justification needs to be in place: How will queries interact with containers or codebases that use std::size_t?
2. Should there be a const_mdspan class to indicate that this is a view that doesn't modify the codomain data?
3. Add using declarations for const_reference and const_pointer?
4. Maybe provide template deduction guides.
5. The co-domain space definition may need to be restricted a bit so that it cannot be interepreted that its elements may be pointers. (They could be with the exception of member-function pointers though).
6. Should there be an additional section describing the allowance of for an mdspan with dynamic number of dimensions for a future addition?

   Section 3.2

7. msspan -> mdspan
8. Should operator() be defined such that it works with std::tuple and std::array? (The constructor would be nice too).
9. Maybe add rank_static?
10. Why is the parameter of the extent member functions int, since it is required it to be >=0?
11. std::extent integral constants are std::size_t. Shouldn't the rank() return type be std::size_t as well?
12. Similarly shouldn't the extent member functions return std::size_t so that it is compatible with std::extent?
13. Should the extent, static_extent and stride be implemented such that they can in addition take the index as a template parameter like: template <std::size_t I> constexpr const size_type &extent(), or this functionallity will be just left to std::extent? Having static access will be necessary for compile-time algorithms.
14. "Extensions to access properties may cause reference to become a proxy type". This is very useful, can it be elaborated a bit more so that implementations do not lock out of such a feature?
15. Shouldn't Section 3.2.2 have the remaining using declarations?
16. Consider changing the name of static_extent. Maybe make it private and introduce a static constexpr bool is_static_extent<I> member function. This will resolve the ambiguity of expressions like "A statically declared extent of dynamic_extent denotes that the extent is dynamic".
17. "If 0 <= r < rank() the extent of coordinate r. If rank() <= r then extent(r) == 1" requires a run-time check. Should an alternative mechanism be proposed or just say If rank() <= r then extent(r) is undefined?
18. Should size() be renamed to element_count() or similar?
19. The size() algorithm should be a non-recursive compile time one.
20. What does the span() function return if is_always_contigous != true?
21. Why should rank_dynamic() <= sizeof...(DynamicExtents) and not rank_dynamic() == sizeof...(DynamicExtents)?
22. V::static_extent(r) == V::static_extent(r) -> V::static_extent(r) == U::static_extent(r)
23. static constexpr bool is_always_unique: what is the meaning of always?
24. The mapping observers can be very specific to the layout and I am not sure they should be exposed by mdspan. For example the stride member function may not be appropriate for user-defined layouts (let's say a symmetric matrix, where the stride depends on the indices)
25. I am not certain that public exposure is needed for required_span_size.
26. size_type is used in 3.2.7 but not defined anywhere.

    Section 3.3

27. The preferred mechanism for declaring rank and static extents is amazing.
28. The DynamicExtents constructor should be disabled for an all-static extents.

    Section 3.4

29. What about static slice specifiers?

    Section 3.5

30. size_type is used in 3.5.2 but not defined anywhere.
31. In 3.5.2, extent in the layout reference interface returns an index_type, but in the description a size_type
32. Previous comments about compile time flavour of relevant member functions applies here too.

Best Regards, Nasos

[hcedwar]

1. justification added

2. note regarding const access when std::is_const<element_type>

3. not applicable

4. to-be-done in subsequent revision

5. done

6. explicitly ruling out dynamic number of dimensions (non-static rank), implementation is not optimizable

7. done

8. array yes, tuple no; want optimizability

9. rank() is rank_static() 10, 11, 12. using signed integer types throughout

10. constexpr function values provides static access as long as the input argument is a literal value

11. Added appendix that references P0019, Atomic Reference as an example.

12. done

13. let' see how it goes in LEWG

14. If r is a literal value or constexpr resolved value then this can be resolved at compile-time because rank() is guaranteed constexpr. Otherwise r is runtime values and a runtime check is necessary.

15. size() is the number of unique elements in the domain, which follows conventional nomenclature

16. An implementation can optimize by pre-compute at construction and saving the value to avoid repeating the iteration on each query.

17. The codomain is defined to be a contiguous span of objects that must include all objects that can be referenced through the mdspan::operator(). The set of referenced objects may be a non-contiguous subset of this span.

18. Clarified why / how

19. done

20. Example, a strided layout could be unique but is not guaranteed to be unique

21. Good discussion. I've pondered exposing the mapping type and object instead of exposing them as a call-through. I added a subsection Mapping Observers - Alternative.

22. Needed if an application is going to allocate a codomain needs to be polymorphic on the layout and dimensions.

23. All should be index_type, fixed.

24. Yes - please advocate for it!!!

25. Hopefully this just goes away...

26. Added requirement and noted to-be-done 29, 30. All should be index_type, fixed.

27. Is all good due to the magical-goodness of constexpr functions with integral values

Much thanks for the detailed review!

[hcedwar]

done in current version