bryancall commented on PR #13552: URL: https://github.com/apache/trafficserver/pull/13552#issuecomment-5328251604
Another round of suppressed Copilot notes, so again there are no threads to reply to. Recording the dispositions. **Took the `stale_response` one** in 2b3c27c. The point about a nullable owning pointer pushing the `PLUGIN_TAG` fallback onto every caller was right, and holding it as a `std::string` is better than either version I had: empty means the default tag, the effective name is computed where it is used, and the destructor no longer owns anything. It also turns the defect that started this from fixed into impossible, since assigning to a string releases the previous value and there is no ownership left to get wrong. **Declined the `uri_signing` one.** The suggestion was to replace the `malloc`/`strcpy` pair for `cfg->id` with `TSstrdup`/`TSfree` for consistency. That file is C style throughout: `config_new()` allocates with `malloc` and `config_delete()` releases with `free`, so switching only the duplication site would create exactly the allocator mismatch the note warns about. Making the whole file consistent is a reasonable cleanup but it is not this change. **Declined the `CacheTool` one again**, same reasoning as last round: encoding ownership in the member types is the better design, but it changes the type of a public member that two unrelated code paths iterate, and I would rather not carry that in a leak fix. The concrete risk it names, a second `build_stripe_hash_table()` call leaking the previous table, is already handled in e860bb7. Verified: Debug build clean with 140/140 unit tests, and a Release build with warnings as errors produces no errors beyond the two that master already produces with this compiler. Note on the CentOS failure on the previous commit: `ci-centos` is the only Release configuration in the matrix and was the only platform that failed, with Debian, Fedora, Rocky and Ubuntu all green on the same commit. I could not reproduce it locally, so I am watching whether this push clears it. -- 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]
