bryancall commented on PR #13552: URL: https://github.com/apache/trafficserver/pull/13552#issuecomment-5328513309
Third round of suppressed notes. Two of them I had declined before, and on a closer look one of those was worth taking after all. **Took the stripe hash table one** in 639cea0. I had been declining this as a type redesign, but there is an in-tree idiom that makes it a two line change: `ats_scoped_mem<T>` in `tscore/ink_memory.h`. Its assignment operator clears the existing resource first, so holding the table in one removes **both** manual frees at once, the destructor release and the free-before-reassign I added in e860bb7. That is a better answer than either version I had, so thanks for pressing on it. **Took the `uri_signing` one**, which changed since last round. Previously it suggested `TSstrdup`/`TSfree`, which I declined because that file allocates with `malloc` and releases with `free` throughout, so switching only the duplication site would have created the very mismatch the note warned about. This round it suggests a plain single-call duplication instead, and `strdup` pairs correctly with the `free` already in `config_delete()`, so `malloc` plus `strcpy` is now just `strdup`. **Took the spelling note.** The tree overwhelmingly writes "null-terminated" (47 occurrences against 2 of the form I used), so the comment now matches. **Still declining the URL set half.** `URLset` is a public member iterated by two code paths that are otherwise untouched here, so converting it to an owning container changes their loops as well. That is a reasonable cleanup on its own and I would rather it be its own change than ride along in a leak fix. Verified: Debug build clean with 140/140 unit tests, and a Release build with warnings as errors produces no errors beyond the two master already produces with this compiler. -- 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]
