Copilot commented on code in PR #3599:
URL: https://github.com/apache/brpc/pull/3599#discussion_r4250134531
##########
src/butil/endpoint.cpp:
##########
@@ -442,13 +442,8 @@ short kqueue_to_poll_events(int kqueue_events) {
}
#endif
-int pthread_fd_wait(int fd, unsigned events,
- const timespec* abstime) {
-#if defined(OS_LINUX)
- const short poll_events = epoll_to_poll_events(events);
-#elif defined(OS_MACOSX)
- const short poll_events = kqueue_to_poll_events(events);
-#endif
+static int pthread_fd_wait_impl(int fd, short poll_events,
+ const timespec* abstime) {
Review Comment:
`pthread_fd_wait_impl` is a good refactor, but the new non-static macOS
overload introduced below (`int pthread_fd_wait(int fd, int events, ...)`) adds
an externally visible symbol that (from this diff) is not declared in a shared
header. This commonly triggers `-Wmissing-declarations`/`-Wmissing-prototypes`
style builds and makes the effective API surface differ across translation
units. Consider either (mandatory) declaring the macOS `int` overload in the
appropriate public header under `#if defined(OS_MACOSX)`, or (alternative)
keeping only the `unsigned` public API and having macOS callers (including
`pthread_timed_connect`) call `pthread_fd_wait_impl(..., POLLIN/POLLOUT, ...)`
directly so no extra exported overload is needed.
##########
test/endpoint_unittest.cpp:
##########
@@ -16,19 +16,30 @@
// under the License.
#include <gtest/gtest.h>
+#include <atomic>
+#include <pthread.h>
+#include <signal.h>
#include <butil/fd_guard.h>
#include <butil/fd_utility.h>
#include "butil/errno.h"
#include "butil/endpoint.h"
#include "butil/logging.h"
+#include "butil/memory/scope_guard.h"
#include "butil/containers/flat_map.h"
#include "butil/details/extended_endpoint.hpp"
#include <netinet/tcp.h>
-#if defined(OS_MACOSX)
+#if defined(OS_LINUX)
+#include <sys/epoll.h>
+#elif defined(OS_MACOSX)
+#include <sys/event.h>
#include <netinet/tcp_fsm.h>
#endif
namespace butil {
+int pthread_fd_wait(int fd, unsigned events, const timespec* abstime);
+#if defined(OS_MACOSX)
+int pthread_fd_wait(int fd, int events, const timespec* abstime);
+#endif
Review Comment:
The test adds manual forward declarations for `butil::pthread_fd_wait`
(including a macOS-only overload). This is brittle: the test can silently drift
from the real declarations/`#if` guards, and it effectively relies on a
non-header-declared symbol. Prefer (mandatory) including the header that
declares `pthread_fd_wait`, and if the `int` overload is intended to be used
outside `endpoint.cpp` then declare it in that header as well; otherwise
(alternative) adjust the macOS tests to exercise the existing `unsigned` API
(e.g., via `static_cast<unsigned>(EVFILT_READ/WRITE)`) without depending on the
overload.
##########
test/run_tests.sh:
##########
@@ -46,6 +46,23 @@ if [ $test_num -eq 0 ]; then
fi
print_bt () {
+ case "${2-}" in
+ ''|*[!0-9]*) return ;;
+ esac
+ # Assertion failures exit normally. A passing death test may have left a
+ # core in this binary, so only consider statuses that can represent a
signal.
+ if [ "$2" -le 128 ] || [ "$2" -gt 255 ]; then
+ return
+ fi
+ # Strip leading zeros so shell arithmetic does not interpret them as octal.
+ signal_number=$2
+ while [ "${signal_number#0}" != "$signal_number" ]; do
+ signal_number=${signal_number#0}
+ done
+ signal_number=$((signal_number - 128))
Review Comment:
`signal_number` is introduced as a function-scoped working variable, but in
shell functions variables are global by default. This can unintentionally
clobber a same-named variable outside `print_bt` (especially in larger
scripts). Consider using a uniquely prefixed variable name (e.g.,
`bt_signal_number`) or, if this script is guaranteed to run under Bash, declare
it as `local signal_number`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
