proost commented on code in PR #521:
URL: https://github.com/apache/datasketches-cpp/pull/521#discussion_r3943402199
##########
hll/include/HllSketchImpl.hpp:
##########
@@ -80,7 +79,6 @@ class HllSketchImpl {
const uint8_t lgConfigK_;
const target_hll_type tgtHllType_;
const hll_mode mode_;
- const bool startFullSize_;
Review Comment:
Removing the trailing `const bool startFullSize_` is a silent ABI break. How
about keeping the member and simply not serializing? it reaches the same stated
goal.
##########
hll/include/HllUtil.hpp:
##########
@@ -43,7 +43,12 @@ static const uint8_t FAMILY_ID = 7;
static const uint8_t EMPTY_FLAG_MASK = 4;
static const uint8_t COMPACT_FLAG_MASK = 8;
static const uint8_t OUT_OF_ORDER_FLAG_MASK = 16;
-static const uint8_t FULL_SIZE_FLAG_MASK = 32;
+// Bit 32 is RESERVED and must not be written or interpreted.
+// It was formerly FULL_SIZE_FLAG_MASK here, while datasketches-java uses the
same bit as
+// REBUILD_CURMIN_NUM_KXQ_MASK for its union gadget. The two meanings collided
across
+// implementations, so this side no longer writes or reads it. Do not reuse:
bits 64 and 128
+// are free.
+static const uint8_t RESERVED_FLAG_MASK_32 = 32;
Review Comment:
Most users don't user it. But if some user use it, then it makes a hard
compile break.
One of more safe choice is introducing new const and make deprecation. And
next release remove it.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
