ARROW-4461: [C++] Expose bit map operations that work with raw pointers#3560
ARROW-4461: [C++] Expose bit map operations that work with raw pointers#3560emkornfield wants to merge 5 commits into
Conversation
fsaintjacques
left a comment
fsaintjacques
left a comment
There was a problem hiding this comment.
Need a small fix in the documentation, the other comments/suggestions are optional and nitpicks.
There was a problem hiding this comment.
While this is just a test, why not usememset?
There was a problem hiding this comment.
Thanks, haven't cached memset into my working repertoire yet.
There was a problem hiding this comment.
This method does not allocate, documentation regarding out_buffer go on previous method.
There was a problem hiding this comment.
Good catch. fixed.
There was a problem hiding this comment.
Lots of duplication (2 functions for each operation), bonus point for a macro:
#define BITMAP_BINARY_IMPL ...
BITMAP_BINARY_IMPL(And, std::bit_and<uint8_t>, std::logical_and<bool>);
BITMAP_BINARY_IMPL(Or, std::bit_or<uint8_t>, std::logical_or<bool>);
There was a problem hiding this comment.
If you don't mind I'm going to leave as is MACROs are discouraged in the style guide and I'm on the fence if this is enough boiler to justify using them.
emkornfield
left a comment
emkornfield
left a comment
There was a problem hiding this comment.
@fsaintjacques thanks for the review. I addressed the issues other then using the MACRO
There was a problem hiding this comment.
If you don't mind I'm going to leave as is MACROs are discouraged in the style guide and I'm on the fence if this is enough boiler to justify using them.
There was a problem hiding this comment.
Good catch. fixed.
Codecov Report
@@ Coverage Diff @@
## master #3560 +/- ##
==========================================
+ Coverage 87.76% 88.63% +0.86%
==========================================
Files 673 544 -129
Lines 82763 73824 -8939
Branches 1069 0 -1069
==========================================
- Hits 72640 65431 -7209
+ Misses 10012 8393 -1619
+ Partials 111 0 -111
Continue to review full report at Codecov.
|
xhochy
force-pushed
the
bin_memory_allocs
branch
from
c5b180b to
54e24d9
Compare
|
Rebased |
emkornfield
force-pushed
the
bin_memory_allocs
branch
from
54e24d9 to
08107f0
Compare
|
@xhochy mind taking a look to see if this can be merged? |
|
I'll take a look now |
5 participants
Precursor to fixing the rest of the boolean computer kernels to not allocate memory.