ORNL / ORNL/cpp-proposals-pub

P2630R3 LWG feedback: 2023/05/24

Open
#381 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
HTML
Stars
29
Forks
26
PR merge metrics
No merged PRs in 30d

Description

Typo: Under 3.1, "24.7" section number [mdspan.submdspan] is wrong. Say where to put it, not a number that might change. Reference via stable names: instead of "after 24.7," after the stable name. (7.3 subclause: It's underneath mdspan, one level down: section 7.4. span is 7.2, mdspan is 7.3. This goes in 7.3.)

Synopsis needs to go into 7.3.2 [mdspan.syn], not a separate synopsis. The synopsis is for the header <mdspan>, so one synopsis for this and existing mdspan content. New synopsis content goes after [mdspan.mdspan].

Could submdspan_mapping be hidden friends, if meant to be called by ADL? They don't need to be named in std namespace. (Christian: We discussed this in LEWG. There are use cases where people use layouts independently of submdspan to build other kinds of containers with layouts. We do call them by ADL.) The issue, though, is that users might accidentally call them namespace-qualfieid std::submdspan_mapping. We want people to call it by ADL only, not qualified. To stop them doing qualified calls, put each specialization inside its class: e.g., inside layout_stride::mapping for the overload taking that type. If people want to provide an overload for their own mapping, they can do that in their own namespace.

Change: make each provided overload of submdspan_mapping a hidden friend. This encourages good design practice for custom implementations, and prevents an error case of picking the wrong overload. Could declare the hidden friends inside the mapping, and define them outside, as a way to share wording for the definition.

(submdspan_extents is not a customization point; it stays put.)

integral-constant-like: LEWG specifically asked us not to use std::integral_constant to represent a compile-time integer, but instead to define a (possibly exposition-only) concept. Change: replace just the last line T() == T::value with && bool_constant<T() == T::value>, to ensure that all those constituent parts are constant expressions. We also need to ensure

&& is_convertible_v<T, T::value_type> && bool_constant<T() == T::value>::value;

Comparison of T to value might not give the same result as converting and comparing. Should we care? We need to say that it's their problem, not ours.

template<typename T, typename U = decltype(T::value)>
concept @_integral-constant-like_@ =
  is_integral_v<U> &&
  ! is_same_v<bool, remove_const_t<T>> &&
  convertible_to<T, T::value_type> &&
  bool_constant<T() == T::value> &&
  bool_constant<U(T()) == T::value>;

(convertible_to is better than is_convertible_v. No benefit to using same_as instead of is_same_v.)

template<typename T, typename U = remove_const_t<decltype(T::value)>> // T::value_type
concept integral-constant-like =
  is_integral_v<U> &&
  !is_same_v<bool, U> &&
  convertible_to<T, U> &&
  equality_comparable_with<T, U> &&
  bool_constant<T() == T::value> &&
  bool_constant<static_cast<U>(T()) == T::value>;

Param 2 "The SliceSpecifier template argument(s) and the corresponding value(s)..." We don't define srcanywhere, yet we use it throughout the clause. We need to introducesrc. (Christian: It's a named parameter of the function.) Say "after the first parameter src" instead of just src`. Do we even need this paragraph? Move it to a Note? Or remove it?

A call to submdspan with an mdspan src returns a new mdspan that views a subset of src determined by the SliceSpecifier template arguments and the values of the ...

Or even move to para 1? (The change isn't quite clear here.)


Add an is-slice-specifier trait to replace the four points of Mandates that recur? Replace is-strided-slice with "is specialization of strided_slice." Minimal change: say "is specialization of strided_slice" instead of introducing is-strided-slice. Tomasz will provide detail on further changes offline.


Change: Make para 3 a Note (on submdspan itself, where the call actually appears -- not here). Also add submdspan_mapping to [contents] para 3, in two places: first sentence, and last on the page (to list of make_error_code and make_error_condition).


Para 4.4:

  • Change: "the number of $S_j$" -> "the number of types $S_j$"
  • Why convertible to size_t and not index_type? (Awkward to specify it with index_type, but the convertibility check could be different.) Change: Try using index_type.
  • Change: "Otherwise" in the second bullet, instead of "or else." Drop the second bullet's is_convertible_v is false.

Regarding the above index_type change: "For each function that takes a parameter named src and a parameter pack named slices" as a naming technique? Yes, probably OK.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Locate the proposal text covering [mdspan.submdspan], [mdspan.syn], [mdspan.mdspan], and [contents], then compare each listed feedback item with the surrounding wording. Resolve the open questions about hidden friends, integral-constant-like, parameter explanations, traits, notes, and index_type before editing. Done means all marked Change items and contents references are reconciled in the paper.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
documentation
Issue type
Documentation
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.