-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
ARROW-6847: [C++] Add range_expression adapter to Iterator #5621
Conversation
Getting the Status on the side is a bit weird. Should we return a |
(also needs rebasing @bkietz ) |
@pitrou returning a for (Result<int> i_result : ints.AsRange()) {
ARROW_ASSIGN_OR_RAISE(int i, i_result);
// loop body
} IMHO, this seems less natural to me than placing the check outside: Status status;
for (int i : ints.AsRange(&status)) {
// loop body
}
RETURN_NOT_OK(status); But neither is a problem. @fsaintjacques ? |
4efb645
to
72469af
Compare
@fsaintjacques @pitrou I've refactored so the range's elements are |
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.
Perhaps you should add some tests for Status and Result (in)equality?
Codecov Report
@@ Coverage Diff @@
## master #5621 +/- ##
==========================================
+ Coverage 89.49% 89.49% +<.01%
==========================================
Files 898 796 -102
Lines 125350 118804 -6546
Branches 1501 0 -1501
==========================================
- Hits 112182 106325 -5857
+ Misses 13157 12479 -678
+ Partials 11 0 -11
Continue to review full report at Codecov.
|
@pitrou I've added inequality and tests @fsaintjacques The |
Not sure to get why you need the iterator to fill a Status object. Anyway, the second loop written by @bkietz for (int i : ints.AsRange(&status)) {
// loop body
} is indeed more natural in C++ than having to check anything in the loop body. |
@JohanMabille the Iterator may return an error status when attempting to retrieve the next sequence element. That error needs to be accessible by the consumer of the Iterator, hence the result/side status loops. |
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.
Very handy work, this will definitively improve the ergonomics. I've restarted the tests, failed due to external errors (github or dockerhub network).
class EqualityComparable { | ||
public: | ||
~EqualityComparable() { | ||
static_assert( |
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 this safe to override the destructor just for a static_assert?
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 default generated destructor is just an empty block like this. I think it's fine, but if you prefer I can replace with void CheckEqualsImpemented()
or so
409fa84
to
927ae0f
Compare
Allows
Iterator<T>
to be used in a range-for loop