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
Add distributed ranges as experimental feature. #1479
base: main
Are you sure you want to change the base?
Add distributed ranges as experimental feature. #1479
Changes from 67 commits
318f723
1ac29d0
8557b72
440e673
2f4897f
9189849
11fdbe7
e371685
cd56589
9e97305
d492476
477b204
0152437
5e43e65
2f84319
bc74b46
658e465
8e943bb
f31af79
35bb6bf
80cf60b
89c92c3
60884a2
ddaf6fe
797498d
03c69f1
1047d4a
db83006
5e98434
c2e3911
9fd9ac0
efaf73b
c968195
976bee4
8aa96ff
54eecda
232bf8e
b6f7c14
969cb75
e46716d
35683c2
60beae2
6e17faa
7251f36
78fdcfe
bdfcbe7
08a5273
093834d
21a321e
e3c6b26
f7c82cc
7a0673b
8c89e36
dcd1702
791d5c2
98b8034
00673f5
86f8da5
e4c19f1
5d46146
9223c15
ee243fb
c09ef4b
9944c46
c90223d
ee47222
5065839
f9c9a99
1236faf
e775386
1d1367a
4e3a8c6
f7149f8
0fdff35
8e3e60d
1fd08e6
88c5181
3830442
798b1e9
648eb43
962e671
8e531af
41e2228
b033088
177daaf
4c5fc67
b0cd793
c3bc4ff
81fb32e
3ca7233
e760ce0
cc69962
b3e15fd
d6f21d3
917eeb7
530380b
f3a1df5
3557043
68f59fe
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.
Is there an intention to keep a
distributed-ranges
active development branch which is distinct from main?If not, this probably should be removed before merge.
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.
There is indeed no such intention. This is a temporary line. Let's keep this comment unresolved until the branch is approved to be merged and I will remove this line that time.
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.
sounds good.
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.
Which specifically C++23 features are needed, and why is C++20 not sufficient? I am afraid that requiring C++23 might further limit the number of people ready to try the feature.
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 a good point. I will look into this and get back to you with a precise answer. As of about 6 months ago, the only C++23 features we were using were
std::ranges::zip_view
andstd::ranges::enumerate_view
. We may be using more now, though.We do have internal versions of these that we could substitute in (or did at some point), so I will investigate how much work it would be to revert to requiring C++20 only.
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.
Which functionality would be lost without these C++23 views? Are these exposed in the public API of distributed ranges, or the use is strictly internal?
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 spent some time yesterday and today drafting a PR (#1702) to switch to C++20 only. This essentially replaces the standard
zip_view
,enumerate_view
, andiota_view
with our own versions.This should work, but there are still some thorny issues to work out. In particular, it appears I wrote the
zip_view
's iterator in a way that doesn't handle const-ness correctly---a user can interact with a constzip_view
just fine, but certain constzip_view
s have conflicting reference and value types. e.g., a const value type but a non-const reference type. This causes errors in the standard library when it looks for a common reference between them as part of the iterator concepts. Fixing this, because of the way thatzip_view
is used internally, will likely require re-writing thezip_view
's iterator, which may end up being quite a bit of work.Given the complexity of
zip_view
and that it's difficult to tell exactly how much work this is going to require, I think we should hold off on supporting C++20 for now.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.
@akukanov , last few days me and @BenBrock were trying to make Distrtibuted Ranges in oneDPL compiling and working with C++20 standard. Lot of things still do not compile and most of functionality will have to be disabled if we want to lower requirements from 23 to 20 standard. We both agreed it is not good to put more work to this now. For people requiring C++20 standard and older g++ there is Distributed-Ranges repo which is based on ranges-v3 and works.
Please approve keeping C++23 requirement.
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.
iota_view
is in C++20. So the problem is with justenumerate_view
andzip_view
, and it has two aspects: available public APIs and internal usage.For the available public APIs, a solution is simple: limit availability of only two APIs to C++23, while the majority would work with C++20.
For the internal use, it is not that simple of course. However a good news is that there is already a "domestic" version of
zip_view
in oneDPL https://github.com/oneapi-src/oneDPL/blob/e3ebb58c1cc97261909c3bb3b47e4b3747be8dde/include/oneapi/dpl/pstl/utils_ranges.h#L146 which can be tried for internal use. Of course the necessary modifications are possible there. We do not haveenumerate_view
, but it should not be hard to make out ofzip
andiota
.So, let's keep looking at this problem. It would also be great if you listed the APIs that require only C++20, and those that require C++23 (including if the implementation needs
zip_view
etc.)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 biggest problem that I have with requiring c++23 for distributed ranges is that this API is an experimental one. That means ideally we want to get feedback from the users. From the past we know that our experimental API is not very popular. Requiring c++23 has (almost) killed our chances to get any feedback.
With regard to
zip_view
complexity. I think @kboyarinov has the implementation. You can ask him (when he is back from vacation) if it's still the case. I believe he will be more than happy to donate one.