-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-6558: [C++] Refactor Iterator to type erased handle #5391
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
Conversation
|
@kszucs Do you think the Ursabot build failures here are related? |
pitrou
left a comment
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 a lot for doing this. Here's a bunch of comments.
cpp/src/arrow/util/iterator.h
Outdated
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.
Can you comment a bit on the implementation? So that the person reading this for the first time doesn't scratch their head.
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.
Will do
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.
Ah, sorry, I wasn't clear. I meant comment on the general design (why we're using type erasure) rather than details of member variables - these can stay of course :-)
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 can go in another PR though :-)
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'll add comments about this in #5221
42b4883 to
f73f4d5
Compare
Codecov Report
@@ Coverage Diff @@
## master #5391 +/- ##
==========================================
+ Coverage 88.58% 89.16% +0.57%
==========================================
Files 950 758 -192
Lines 126207 110762 -15445
Branches 1495 0 -1495
==========================================
- Hits 111803 98756 -13047
+ Misses 14039 12006 -2033
+ Partials 365 0 -365
Continue to review full report at Codecov.
|
|
I have not seen the failures, everything seem to pass now though. |
pitrou
left a comment
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.
+1
To resolve linkage issues with MSVC and the current abstract base class Iterator
@pitrou