Jens-G commented on PR #3854: URL: https://github.com/apache/thrift/pull/3854#issuecomment-5688315512
### Code review Found 1 issue: 1. There is no JIRA ticket for this change, and the PR title and the commit subject do not start with `THRIFT-NNNN:`. The change adds a method to the public `TInputProtocol` trait and changes how the binary and compact protocols skip string and binary fields, so it is not a trivial one (AGENTS.md says a ticket is "Required for all non-trivial PRs" and "All significant changes need a JIRA ticket.") https://github.com/apache/thrift/blob/7d6be866ba569aed496b2f263f6278cf747b1bf7/AGENTS.md#L27-L29 https://github.com/apache/thrift/blob/7d6be866ba569aed496b2f263f6278cf747b1bf7/AGENTS.md#L48-L50 One suggestion, below the bar for the list above but verified: - None of the new tests notices if `skip()` goes back to `read_bytes()`, or if the `skip_binary` forward in `Box<P>` or in `TStoredInputProtocol` is dropped, because both paths consume the same bytes and return the same errors. A test transport that records the largest buffer it is asked to fill does notice: while skipping a 1024-byte string it sees a single 1024-byte request on the old path and no request over 256 bytes on the new one. I checked this against all three changes. https://github.com/apache/thrift/blob/7d6be866ba569aed496b2f263f6278cf747b1bf7/lib/rs/src/protocol/mod.rs#L1213-L1224 https://github.com/apache/thrift/blob/7d6be866ba569aed496b2f263f6278cf747b1bf7/lib/rs/src/protocol/mod.rs#L407-L411 🤖 Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- 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]
