jmalkin commented on a change in pull request #186:
URL: https://github.com/apache/datasketches-cpp/pull/186#discussion_r573584317
##########
File path: hll/include/HllUnion-internal.hpp
##########
@@ -226,7 +201,7 @@ HllSketchImpl<A>*
hll_union_alloc<A>::copy_or_downsample(const HllSketchImpl<A>*
return src->copyAs(HLL_8);
}
typedef typename std::allocator_traits<A>::template
rebind_alloc<Hll8Array<A>> hll8Alloc;
Review comment:
Surprised this wasn't replaced with using :)
##########
File path: hll/include/Hll4Array-internal.hpp
##########
@@ -315,12 +316,12 @@ void Hll4Array<A>::shiftToBiggerCurMin() {
template<typename A>
typename HllArray<A>::const_iterator Hll4Array<A>::begin(bool all) const {
- return typename HllArray<A>::const_iterator(this->hllByteArr, 1 <<
this->lgConfigK, 0, this->tgtHllType, auxHashMap, this->curMin, all);
+ return typename HllArray<A>::const_iterator(this->hllByteArr.data(), 1 <<
this->lgConfigK, 0, this->tgtHllType, auxHashMap, this->curMin, all);
Review comment:
I feel like we should probably make the iterator take the std::vector at
some point.
##########
File path: hll/include/HllSketchImplFactory.hpp
##########
@@ -157,8 +159,9 @@ Hll6Array<A>* HllSketchImplFactory<A>::convertToHll6(const
HllArray<A>& srcHllAr
template<typename A>
Hll8Array<A>* HllSketchImplFactory<A>::convertToHll8(const HllArray<A>&
srcHllArr) {
const int lgConfigK = srcHllArr.getLgConfigK();
- typedef typename std::allocator_traits<A>::template
rebind_alloc<Hll8Array<A>> hll8Alloc;
- Hll8Array<A>* hll8Array = new (hll8Alloc().allocate(1))
Hll8Array<A>(lgConfigK, srcHllArr.isStartFullSize());
+ using Hll8Alloc = typename std::allocator_traits<A>::template
rebind_alloc<Hll8Array<A>>;
+ Hll8Array<A>* hll8Array = new
(Hll8Alloc(srcHllArr.getAllocator()).allocate(1))
Review comment:
I'm always torn on whether we should define `auto srcAllocator =
srcHllArr.getAllocator()` in cases like this since we use it twice and it gets
a little verbose to read. Two uses like this is sort of a wash I guess?
It's perfectly fine as is, I'm just not sure if we should have a preferred
style?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
