bryancall commented on PR #13552: URL: https://github.com/apache/trafficserver/pull/13552#issuecomment-5328760387
Fourth round of suppressed notes. I am declining all four, and I want to be specific about why rather than just saying no. **`reset(...)` on the stripe hash table.** `ats_scoped_mem` does not have a `reset()`. The public API of `ats_scoped_resource` is `clear()`, `get()`, `release()`, and `operator=`, so assignment *is* the ownership transfer API for this type. The suggestion cannot be applied as written. **Owning container for `URLset`.** Fourth time for this one and my answer is unchanged: `URLset` is a public member iterated by two code paths this change does not otherwise touch, so converting it edits their loops too. Still worth doing, still not in a leak fix. **Unchecked `TSTextLogObjectCreate`.** This one is legitimate and already identified. It was triaged out of this pull request deliberately: adding a failure branch and an error log is a behavior change, and this pull request is scoped to leaks whose fix is not observable. It is sitting in the behavioral batch with the other changes of that kind, and it will come with an error path rather than as a drive-by. **`strdup` portability.** This contradicts the previous round, which asked for exactly this change: it suggested a single-call duplication and I used `strdup`. On the substance, plain `strdup` already has 11 call sites across `src/` and `plugins/`, and this file allocates with `malloc` and releases with `free` throughout, so `strdup` is both consistent locally and established in the tree. More generally, this is the fourth automated review pass on this pull request and the notes have moved from real defects to preferences about the shape of the fixes, including one that contradicts an earlier round and one that targets an API the type does not have. The earlier passes were genuinely valuable and caught a real regression I introduced. This one is not moving the code forward, and every push re-runs the full matrix, so I am going to leave the code here and let human review drive from this point. State: all checks green on the current head, with the Release build (the only one in the matrix, on CentOS) passing. -- 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]
