Jens-G commented on PR #3446:
URL: https://github.com/apache/thrift/pull/3446#issuecomment-4392288197
### Code review
Found 3 issues:
1. `ServerSocket::accept()` returns `''` instead of `undef` on the failure
path โ `return undef;` is deleted rather than replaced with bare `return;`
In Perl, removing `return undef;` causes the sub to return the last
evaluated expression (the `if` condition), which is `''` (empty string) on the
false path. Since `defined('')` is true, callers that check `if (defined
$client)` (e.g. `Server.pm`) will treat a missing handle as a valid client
object. The fix is `return;` rather than deleting the line entirely.
https://github.com/apache/thrift/blob/f1989fe25eccfeea015c6e0b8c7e7f001c98c99f/lib/perl/lib/Thrift/ServerSocket.pm#L90-L94
2. Secondary `package` declarations in multi-package files are missing `##
no critic (RequireFilenameMatchesPackage)` โ the CI scan will fail on them
`Exception.pm` annotates `Thrift::TException` but not
`Thrift::TApplicationException`. `Transport.pm` annotates
`Thrift::TTransportException` but not `Thrift::TransportFactory` or
`Thrift::ServerTransport`.
https://github.com/apache/thrift/blob/f1989fe25eccfeea015c6e0b8c7e7f001c98c99f/lib/perl/lib/Thrift/Exception.pm#L43-L47
https://github.com/apache/thrift/blob/f1989fe25eccfeea015c6e0b8c7e7f001c98c99f/lib/perl/lib/Thrift/Transport.pm#L131-L135
https://github.com/apache/thrift/blob/f1989fe25eccfeea015c6e0b8c7e7f001c98c99f/lib/perl/lib/Thrift/Transport.pm#L158-L162
3. Before squashing, consider adding a `Client:` line to the commit message
โ the project convention ([AGENTS.md
ยง2](https://github.com/apache/thrift/blob/master/AGENTS.md#L34-L38)) is
`Client: cpp,perl` for changes spanning the compiler and Perl library.
๐ค 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]
