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
New submdspan CPP 17 compatible #227
New submdspan CPP 17 compatible #227
Changes from 17 commits
e004a42
8cfc22f
9c97013
4bf8a09
4defa5e
39450c1
aefb6e5
f516e85
ce9c272
f558537
8a4586b
79c1232
55c9979
4749d80
c8e33dc
8ff29f8
fa91897
36b9376
471ecbc
d7f2b3e
a08fc05
277bb26
7b80df4
15899dc
98c6086
b4a7aeb
c99e6e5
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.
I've seen this with NVCC + GCC 10. The GCC intrinsic function
__builtin_unreachable();
has helped me get rid of the warnings in this case. C++23 addsstd::unreachable
, which hopefully will accomplish the same goal.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.
Does that GCC intrinsic also work with nvc++, clang, msvc as host compiler?
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.
It's a GCC-only thing, so when I used it, I defined it as a macro.
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.
GCC 10 isn't quite as smart as GCC 11 about
constexpr auto
functions that have exhaustiveif constexpr
branches in them.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.
Thanks for figuring this out! : - )
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.
More of a code readability issue -- the slices and new extent parameters here are kind of mixed up into the same parameter pack so it's difficult to understand the code. Might be worth separating out the new extents into a separate parameter.
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.
The code as it stands won't achieve the goal of
divide
, which is to returnintegral_constant
if both inputs areintegral_constant
.integral_constant<size_t, Value>
results insize_t
, notintegral_constant<size_t, Value+1>
.integral_constant
resulted inintegral_constant
, the result of the ternary operator would becommon_type_t<integral_constant<size_t, Value>, int>
, which isint
.https://godbolt.org/z/qWEdn5xM6
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 doesn't matter since it doesn't determine the type. Note the values I am generating here are used as constructor arguments for an extents type which is independently determined. In case static extents are generated or preserved, these values will at best be checked against the static_extents in the extents constructor.
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.
integral_constant<T, Value>
implicitly converts toT
. This means that1 + integral_constant<size_t, X>{} / integral_constant<size_t, Y>{}
just returns1 + X / Y
, so you don't even needdivide
here.https://godbolt.org/z/x48bnaW8e