Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Host/Device accessors for
mdspan
#3686base: main
Are you sure you want to change the base?
Host/Device accessors for
mdspan
#3686Changes from 5 commits
b115039
0ea452d
9ae4953
96f8419
52b12a6
cab748e
31b84c7
430e46d
3c76d32
d63980d
e9ce8a4
de4264c
c0d12da
572447d
0eb90ef
71a9161
921273a
aef9602
0647703
c30482e
fc82009
21e2feb
22fecf3
ca47b44
9a7e393
a0af8f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessors are not required to be default constructible. Please see https://eel.is/c++draft/mdspan.accessor.reqmts for the actual requirements, and https://eel.is/c++draft/views.multidim#mdspan.mdspan.cons-1.4 for how mdspan constrains its default constructor on whether its accessor is default constructible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty easy to fix in C++20. I have to remind myself how the work-around works in C++ < 20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to fix it. Without c++20, I cannot introduce a template parameter and use
= default
.Also, I don't know what is the best way to use
requires
before c++20, without introducing a template parameter @misccoThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A key feature of any accessor is the set of permitted conversions, both implicit and explicit. mdspan depends on accessors' conversions in order for its own conversions to work. Implicit conversions make mdspan assignment work; explicit conversions make explicit construction of an
mdspan
of one type from an mdspan of another type work. Evendefault_accessor
has a conversion to permit assigning an mdspan of nonconst to an mdspan of const.An accessor's conversions are defined by its constructors and/or conversion operators. Not exposing those from the parent class means that mdspan of
host_accessor<Accessor>
won't be able to perform any of the conversions that mdspan ofAccessor
can perform.A public
using _Accessor::_Accessor;
declaration would pull in all of the base class' constructors, but it wouldn't pull in any conversion operators.Another issue is that
_Accessor
might have a virtual destructor. That would be weird, but it's legal in the Standard and nothing in this class prevents it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Section 5.8 of P2897R7 elaborates the design intent behind accessor conversions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, if Accessor A provides a conversion operator to B and a conversion operator to C, the only way to provide those in
device_accessor<A>
would be to use reflection to enumerate all conversion operators of A, and paste new ones intodevice_accessor<A>
fordevice_accessor<B>
anddevice_accessor<C>
. While I'm not a C++ reflection expert, I don't think even P2996 reflection can do that, as it would require injecting member function definitions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I entirely missed that part. I added the constructors following the
aligned_accessor
model.