cmcfarlen commented on PR #13616: URL: https://github.com/apache/trafficserver/pull/13616#issuecomment-5514386560
Went through all six. Two were already addressed, three are the same point and are now fixed, and one is incorrect — details below. **Fixed in ef97aa66e4** `create()` reimplementing the flag clear, and the duplicated `~UNLISTED` mask (three of the comments): `create()` now calls `set_listed(it->second, true)`, so the mask appears in exactly one place and there is one code path for relisting. On the null-blob concern: a name present in `_lookups` always names an allocated slot, so neither the old inline check nor the new `allocated()` gate can fail there — but routing through one function means the failure modes cannot drift apart, which was the substance of the comment. `current()` returning `std::pair<int16_t, int16_t>` built from two `uint16_t` members is now `std::pair<uint16_t, uint16_t>`. The values are small enough that the narrowing never mattered, but see below for why it was worth removing. **Not a defect: the negative-offset concern** > `entry` comes from `_splitID(id)` as an `int16_t` (low 16 bits). If those low bits are >= 0x8000, `entry` becomes negative and will pass `entry >= MAX_SIZE` `_splitID` returns `std::tuple<uint16_t, uint16_t>`, so `entry` is unsigned. Low bits of `0xFFFF` give `entry == 65535`, which fails `entry >= MAX_SIZE` and is rejected. There is no signed value and no negative subscript. The existing test for `std::numeric_limits<IdType>::max()` covers exactly this input — its offset bits are `0xFFFF`, high bit set — and asserts both `unlist` and `listed` return false. The `int16_t` in the comment is real, though, just in a different function: `current()`, which is what I changed above. That is very likely where the reading came from, so removing the narrowing is worth it even though `allocated()` was never affected. **Already addressed** The `Storage::tombstone()` allocated-slot comment was fixed in 9ed3ee871d, which is what introduced `allocated()`. The PR description mismatch was real when filed; the description now documents `unlist`/`relist`/`listed` and includes a section on why the name changed. No compatibility alias: `tombstone` was never released, and it is the wrong word for this — a tombstone marks something deleted and reusable, which is how `CacheShm` in this same tree uses it. Nothing is deleted here. -- 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]
