Introduce beginnings of prototype/reference implementation, adjust wording #55

hcedwar · 2018-07-07T00:45:08Z

Introduce beginnings of prototype / reference implementation based upon updating prior prototypes to new wording.
Adjust wording based upon prototyping experience.
Non-trivial change to the Accessor to allow a non-template Accessor class to denote access properties independent of the element type.

wording based upon prototyping experience.

crtrott

Hi Carter, I put in some comments based on our review of the pull request. There are number of points we probably need to discuss. Let me know if you think its best to have a telecon.

crtrott · 2018-07-09T21:46:52Z

P0009/P0009.bs

@@ -1805,28 +1796,16 @@ namespace std {
 namespace experimental {
 namespace fundamentals_v3 {

-template<class T>
 struct accessor_basic {


I believe this is not a good choice. Besides the fact I find it a bit awkward (but thats my "I like C++ usage as C with classes" inner self ;-) ), this would make it more cumbersome to provide accessors which are partially or fully specialised. The original arguments is whether accessor should be defined in terms of a nested class like the layout. We felt here that it would make the language definition more cumbersome, for not much gain. The template argument being templates is something which happens throughout C++ anyway (for example allocators for containers). Thoughts?

The gain is to be able to define and compose access properties independently of the type to which they are applied; e.g., atomic, streaming, etc. Cumbersome to compose access properties if the ElementType must be carried along during composition, as apposed to being applied after the fact. Consider a "rebind" or "apply to" interface:
auto ax = apply_property( x , access_atomic );
where
inline constexpr const access_atomic_t access_atomic;

I don't see how it's any more awkward or cumbersome to include an element type. You have:

auto ax = apply_property(x, access_atomic<T>);

where

template <typename T> inline constexpr access_atomic_t<T> access_atomic;

or even just:

auto ax = access_atomic(x);

Actually, the code you gave originally:

auto ax = apply_property( x , access_atomic );

can just work as is, assuming an implementation of apply_property that deduces the type of x and acts on some sentinel access_atomic, especially if we add an optional rebind member template, like on allocator. I wouldn't be opposed to that, because that's something I think we could reasonably allow at the concept level without losing the ability to allow accessors to restrict the types they act on and partially specialize.

crtrott · 2018-07-09T21:47:38Z

P0009/P0009.bs

@@ -1112,7 +1110,7 @@ struct layout_left {
      constexpr mapping& operator=(const mapping&lt;OtherExtents>& other);

    // [mdspan.layout.left.ops], layout_left::mapping operations
-    Extents get_extents() const noexcept;


The rename from get_extents to extents is fine, and more in line with what the standard does (there are not many classes with get_.. members). Returning const Extents& is more problematic we believe, since it probably now requires that the mapping stores an extent. Before it could store something else which could be used to create an extent.

at least weaken to convertable to Extents so an implementation is not forced to have the penalty of constructing an extent every time the function is called.

From the CppCoreGuidelines:

What is “cheap to copy” depends on the machine architecture, but two or three words (doubles, pointers, references) are usually best passed by value.

Most extents will be smaller, so our concrete types should return by value IMHO. But I agree, the concept should not have this constraint.

Doesn't that mean as in the case below that we end up having problems with stuff like auto on the return type of extents? Also convertable to Extents doesn't solve the issue of creating a copy. I either return a reference to something that has to exist in mapping (in which case changes to mapping would be observable in the returned reference I got via auto& e=m.extents(); , or I return a copy in which case I don't need to prescribe what is in mapping.
So two options:

return const Extents& and mapping has now a real member of Extents not just exposition only, but it fixes the "I need to create a copy" issue.

leave it as is.

Thoughts on what is preferable?

crtrott · 2018-07-09T22:00:27Z

P0009/P0009.bs

-<b>26.7.�.2.1 `accessor_basic` operations [mdspan.accessor.basic.ops]</b>
-
-```c++
-constexpr reference operator()(pointer p, ptrdiff_t i) const noexcept;


We think it is okish to delete that operator, but there are drawbacks and benefits:

benefit: common case, pointer is just T* has now one less inlined function.

drawback: requires pointer to be more often a wrapper object which contains all the stuff the accessor would contain, for example for the atomic accessor, pointer is not anymore T*. It needs to be some wrapper class which is convertible to T* and which has an [] operator.

drawback: pointer must be templated on reference, since its operator [] must return that.

drawback: no clear separation of access and storage.

e.g. if I want to use some intrinsic to access the data in previous solution that intrinsic is in the operator of the accessor, now I write a little class containing a pointer and a [] operator, and the implementation of the [] operator contains the intrinsic

It seems this makes stuff more complex down the road. My main issue is that pointer[int] must return a reference, which means pointer and reference are not independent any more.

Yep - not independent. The pointer and reference should have a strong relationship. The pointer is carried around as a member of mdspan and is handle to storage, the reference is an associated handle to accessing an element of that storage. Should just be able to copy the storage handle and everything works - no extra wrappers.
See the prototype where template<class T> using pointer = T *; for the accessor_basic.

crtrott · 2018-07-09T22:05:59Z

P0009/P0009.bs

  <td></td>
-  <td>*Requires:* `iterator_traits<A::pointer>::reference` shall be convertible to `A::reference` </td>
+  <td>*Requires:* `A::pointer<T>` shall be `DefaultConstructible` and `CopyAssignable`. *[Note:* This may be `T*` or a proxy that operates like `T*`. *--end note]*</td>


This doesn't seem to be enough in terms of what pointer is. This for example doesn't define that operator[] (ptrdiff_t) exists or how that behaves. It doesn't define that operator* () exists or what it does. And we need arithmetic on those to allow subspan. If we define all of those than we are probably getting really close to random access iterator. Is there anything specific in random access iterator we don't want or shouldn't require for pointer?

Operational spec required p[i] to return a reference to the offset - so that is still there.
An iterator also requires *p, ++p, and p++ - which should not be required by operational spec.

The point about *p, ++p, and p++ not needing to exist is interesting. Do you envision any cases where this would be necessary? You're losing a lot of the power you get by separating storage from access, and it's not clear how you would form the codomain span with something that doesn't meet the requirements of RandomAccessIterator.

crtrott · 2018-07-09T22:09:02Z

P0009/P0009.bs

-
-  </td>
+  <td>`m.extents()`</td>
+  <td>convertable to `E`</td>


Is there a good reason to not return an actual Extent? We'd rather have the more specific one. vector is a good example where this is a problem, since it returns something convertible to bool and not a bool. Which means that for example doing auto and than using the variable in a templated function is an issue.

Convertable allows return by value or return by constant reference, which allows an implementation to avoid the overhead of being required to construct and return a copy.

crtrott · 2018-07-09T22:16:11Z

P0009/P0009.bs

+
+```c++
+template<class... IndexType>
+static constexpr index_type required_span_size(IndexType... dynamic_extents) noexcept;


We discussed this and were not in favor of replicating too much stuff in mdspan which exists in mapping. But it wasn't a super strong preference. If you like it to be in here that is fine.

It's not just about replicating too much stuff. This won't work for non-unique, stateful mappings, so the operational semantics would have to somehow exclude those mappings, and the mapping would have to somehow advertise that its statefulness affects its required span size. I'm okay with putting it back in if those problems are solved, but this pull request doesn't solve those problems AFAICT.

The mapping version requires construction of a mapping object and then function call on that object, a syntactically long two-step operation . I believe this will be a frequent pattern and the shortcut is a desirable convenience. The other single-step operations could be removed.

crtrott · 2018-07-09T22:18:48Z

P0009/P0009.bs


 <br/>

-```c++
-constexpr index_type size() const noexcept;


We believe we need size and unique_size. One example is I want to pack a strided view into an MPI buffer. I need to know how large to make that buffer, and its not required_span_size. And if we add size we probably also need a function which returns something discounting all the replicated entries in the mapping.

Pull them from the mapping :-)

The reasoning for having size() and unique_size here is for consistency with container-like types (this is going into the containers subclause of the standard after all) in the case of size(), and for clarity as to which size the size() method refers to in the case of unique_size.

…n behavior.

…cessor policy.

crtrott · 2018-08-26T06:42:32Z

Closing this now. With the just merged commit of a reference implementation we should have gotten everything transferred from here.

Carter Edwards and others added 2 commits July 6, 2018 17:07

Introduce updated prototype according to current P0009, and adjust

3a0cd15

wording based upon prototyping experience.

Allow prototype to build with C++11

d67b518

hcedwar requested review from dsunder and crtrott July 7, 2018 00:45

crtrott requested changes Jul 9, 2018

View reviewed changes

hcedwar added 2 commits July 11, 2018 10:32

Simplify accessor in prototype

51e8133

Update wording for simplified accessor and review with co-authors.

2782a5d

hcedwar mentioned this pull request Jul 11, 2018

Accessor abstraction (PR #55) #56

Closed

hcedwar added 5 commits July 11, 2018 12:24

Align prototype with accessor wording.

d74603b

Finish replacement of pointer with accessor.

970ad47

Accessor constructor may impose accessor-dependent restrictions.

cfe9185

Continued refinement of accessor abstraction. Clarification of subspa…

5bad141

…n behavior.

Clean up for bikeshed. Further simplification of subspan wording.

9852aa6

hcedwar mentioned this pull request Jul 12, 2018

P0009: static extent preservation in subspan can't be QOI because of ABI compatibility #57

Closed

hcedwar added 9 commits July 12, 2018 12:41

Fix bikeshed formatting mistakes

3a9dc66

Example accessor for atomic_ref.

f0f30de

Replace the accessor template argument and accessor traits with an ac…

4ca358c

…cessor policy.

Fix bikeshed syntax errors

f3e5196

Clean up.

817869b

Fix cut-n-paste mistake

f1b9f6a

Fix bikeshed formatting fix

7fcf8f0

Update prototype as per accessor policy iteration.

7e3f0e8

Add example extension of accessor policy for std::shared_ptr.

af7fd91

hcedwar mentioned this pull request Aug 9, 2018

Traditional access a[i][j]k] seems missing #60

Open

crtrott closed this Aug 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce beginnings of prototype/reference implementation, adjust wording #55

Introduce beginnings of prototype/reference implementation, adjust wording #55

hcedwar commented Jul 7, 2018

crtrott left a comment

crtrott Jul 9, 2018

hcedwar Jul 9, 2018

dhollman Jul 9, 2018 •

edited

Loading

dhollman Jul 9, 2018

crtrott Jul 9, 2018

hcedwar Jul 9, 2018

dhollman Jul 9, 2018

crtrott Jul 9, 2018

crtrott Jul 9, 2018

hcedwar Jul 9, 2018

crtrott Jul 9, 2018

hcedwar Jul 9, 2018

dhollman Jul 9, 2018

crtrott Jul 9, 2018

hcedwar Jul 9, 2018

crtrott Jul 9, 2018

dhollman Jul 9, 2018

hcedwar Jul 9, 2018

crtrott Jul 9, 2018

hcedwar Jul 9, 2018

dhollman Jul 9, 2018

crtrott commented Aug 26, 2018

Introduce beginnings of prototype/reference implementation, adjust wording #55

Introduce beginnings of prototype/reference implementation, adjust wording #55

Conversation

hcedwar commented Jul 7, 2018

crtrott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhollman Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crtrott commented Aug 26, 2018

dhollman Jul 9, 2018 •

edited

Loading