leerho commented on code in PR #522: URL: https://github.com/apache/datasketches-cpp/pull/522#discussion_r3971956263
########## 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: What you see in this diff is stale, and that's on me — #522 is stacked on the pre-fix commit of #521 and hasn't been brought up to date with master yet, so the file view still shows the old state. What you don't see is the change made in #521, which addressed this same comment. On master today: ``` static const uint8_t RESERVED_FLAG_MASK_32 = 32; // Deprecated alias for the bit above, retained for source compatibility. Do not use. // (No [[deprecated]] attribute: this library targets C++11, where it is unavailable.) static const uint8_t FULL_SIZE_FLAG_MASK = RESERVED_FLAG_MASK_32; ``` I'll merge master into this branch and it will drop out of the view. **On removal:** with C++11 there's no [[deprecated]], so the alias warns nobody — it's purely a source-compatibility placeholder. Rather than name a version we can't predict yet, I'll record the date and PR in the comment itself so it can be traced to whatever release follows. Intent is to drop it one release after it ships. It will look something like this: ``` static const uint8_t RESERVED_FLAG_MASK_32 = 32; // Deprecated alias for the bit above, retained for source compatibility. Do not use. // Added 2026-09-07 (#521), first shipping in the release after 5.2.0. No [[deprecated]]: // this library targets C++11, so the alias warns no one. static const uint8_t FULL_SIZE_FLAG_MASK = RESERVED_FLAG_MASK_32; ``` -- 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]
