leerho commented on PR #521: URL: https://github.com/apache/datasketches-cpp/pull/521#issuecomment-5561043187
Thanks for the careful review — this is exactly the right thing to be checking, and you're right on the second point. I've pushed a fix for it. On the first point I think the measurements argue the other way; details below. --- ## `FULL_SIZE_FLAG_MASK` → `RESERVED_FLAG_MASK_32` — agreed, fixed You're right, and this is the one break in the PR that costs nothing to avoid. It's also the only *loud* one: it breaks the build rather than misbehaving quietly. Fixed in eeb8d1f by keeping the old name as an alias: ```cpp 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 couldn't use `[[deprecated]]` as you suggested — the library is `CMAKE_CXX_STANDARD 11` and that attribute is C++14 — so the deprecation is a comment. With this in, the PR introduces no source break at all. ## Removing `startFullSize_` — I'd like to keep it removed **On the layout claim: you're right that something shifts, though not in the classes one would expect.** From Clang's `-fdump-record-layouts`, before vs. after: | class | `sizeof` | member offsets | |---|---|---| | `HllSketchImpl<A>` | 24 → 24 | dsize 21 → 20 | | `HllArray`, `Hll4/6/8Array` | unchanged | **unchanged** (own members are 8-byte-aligned doubles; still start at 24) | | `CouponList<A>` | 56 → 56 | `couponCount_` **24 → 20**, `oooFlag_` **28 → 24** | | `CouponHashSet<A>` | unchanged | same 4-byte shift | So there is a real shift, in the coupon classes, and `sizeof` hides it. **But it can't be observed.** Every accessor on `CouponList` / `CouponHashSet` is `virtual` (`CouponList.hpp` lines 43–79) — there is no non-virtual inline member access anywhere in the hierarchy. Member reads therefore only ever happen inside one link-time-selected definition, and since `sizeof` is unchanged, allocation and deallocation sizes still match. I linked a TU built against `master` headers against a TU built against this branch, at `-O0`, `-O2` and `-O3`, in both link orders: identical results every time, no corruption. The shift is unreachable by construction, not by luck. **On "keeping the member reaches the same goal" — I implemented your suggestion and measured it.** Keeping the member, the ctor and `isStartFullSize()`, and only dropping the read/write of bit 32: | | master | keep-the-member | this PR | |---|---|---|---| | born full-size → `reset()` | full-size | full-size | LIST | | **same sketch round-tripped → `reset()`** | full-size | **LIST** | LIST | | **round-trip consistent?** | yes | **no** | yes | | bit 32 written | `0x20` | no | no | | **union fed a full-size sketch → `reset()`** | full-size *(bug)* | **full-size *(bug remains)*** | LIST *(fixed)* | Two problems with the minimal version. It doesn't fix the union bug, which is the PR's second stated goal — a union that has had a full-size sketch merged into it still resets its gadget to full-size. And it introduces a *new* silent inconsistency: a sketch and its own deserialized clone now disagree about what `reset()` does. Removing the persistence of a sticky flag while keeping the flag makes behaviour depend on unserialized history, which seems to me squarely in the "easy to miss" category you're trying to avoid. **On scope: keeping the member addresses 1 of the 12 changed symbols.** Diffing emitted symbols for the default allocator, this PR removes: - `HllSketchImpl::isStartFullSize()` ← the only one from the data member - `HllSketchImpl::reset()`, `hll_sketch_alloc::reset()` - `HllSketchImpl`, `HllArray`, `Hll4Array`×2, `Hll6Array`×2, `Hll8Array`×2 ctors - `HllSketchImplFactory::newHll(..., bool, ...)` The other 11 are signature changes, and they're all loud. (Note `hll_sketch_alloc::reset()` → `reset(bool = false)` is source-compatible for every caller; it only breaks a pointer-to-member.) ## On ABI policy generally Worth separating from this PR: every target in the build is `add_library(... INTERFACE)` — 15 of them, header-only, no SONAME, no compiled artifact, no explicit template instantiations. In that model any change to any inline function body is already an ODR hazard for anyone mixing header versions in one link, so the practical rule has always been "recompile everything," and I don't think this PR changes the exposure. There's also precedent already on master for 5.3.0 — private data members removed from public template classes that shipped in 5.2.0: - `tdigest<T,A>::internal_k_` and `::buffer_capacity_` (315e50b) - `theta_update_sketch_base::buf_i_` (eb3200e) Same class of change. I'd rather not hold `startFullSize_` to a stricter standard than those unless we adopt a policy for the whole library — which might be a good dev@ thread, but probably shouldn't be settled per-PR. Happy to be talked round on any of this, especially if you can see a way to fix the union bug that keeps the member. -- 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]
