Copilot commented on code in PR #3670:
URL: https://github.com/apache/brpc/pull/3670#discussion_r4377679480
##########
test/endpoint_unittest.cpp:
##########
@@ -308,6 +308,25 @@ TEST(EndPointTest, original_endpoint) {
ASSERT_EQ(ep.port, ep2.port);
}
+TEST(EndPointTest, self_assignment) {
+ const char* addresses[] = {
+ "0.0.0.0:0", "1.2.3.4:5678", "[::1]:8080", "unix:sock.file"
+ };
+ for (const char* address : addresses) {
+ SCOPED_TRACE(address);
+ butil::EndPoint ep;
+ ASSERT_EQ(0, butil::str2endpoint(address, &ep));
+ ExtendedEndPoint* eep = ExtendedEndPoint::address(ep);
+ const butil::EndPoint& alias = ep;
+ ep = alias;
+ ASSERT_STREQ(address, butil::endpoint2str(ep).c_str());
+ ASSERT_EQ(eep, ExtendedEndPoint::address(ep));
Review Comment:
`ExtendedEndPoint::address(ep)` is called unconditionally. In the
surrounding codebase (e.g., `endpoint2str`) it’s only called after
`ExtendedEndPoint::is_extended(point)` succeeds, which strongly suggests
`address()` may only be valid for extended endpoints. Please guard this call
with `is_extended(ep)` (otherwise this test can be UB/flaky/crash on IPv4
cases).
##########
test/endpoint_unittest.cpp:
##########
@@ -308,6 +308,25 @@ TEST(EndPointTest, original_endpoint) {
ASSERT_EQ(ep.port, ep2.port);
}
+TEST(EndPointTest, self_assignment) {
+ const char* addresses[] = {
+ "0.0.0.0:0", "1.2.3.4:5678", "[::1]:8080", "unix:sock.file"
+ };
+ for (const char* address : addresses) {
+ SCOPED_TRACE(address);
+ butil::EndPoint ep;
+ ASSERT_EQ(0, butil::str2endpoint(address, &ep));
+ ExtendedEndPoint* eep = ExtendedEndPoint::address(ep);
+ const butil::EndPoint& alias = ep;
+ ep = alias;
+ ASSERT_STREQ(address, butil::endpoint2str(ep).c_str());
Review Comment:
Comparing `endpoint2str(ep)` to the original input string can be overly
strict if `endpoint2str` normalizes formatting (e.g., IPv6 canonicalization,
Unix endpoint formatting differences). To make the regression resilient while
still validating correctness, prefer comparing `endpoint2str(ep)` before vs
after self-assignment (or comparing structured fields for non-extended
endpoints).
##########
test/endpoint_unittest.cpp:
##########
@@ -308,6 +308,25 @@ TEST(EndPointTest, original_endpoint) {
ASSERT_EQ(ep.port, ep2.port);
}
+TEST(EndPointTest, self_assignment) {
+ const char* addresses[] = {
+ "0.0.0.0:0", "1.2.3.4:5678", "[::1]:8080", "unix:sock.file"
+ };
+ for (const char* address : addresses) {
+ SCOPED_TRACE(address);
+ butil::EndPoint ep;
+ ASSERT_EQ(0, butil::str2endpoint(address, &ep));
+ ExtendedEndPoint* eep = ExtendedEndPoint::address(ep);
+ const butil::EndPoint& alias = ep;
+ ep = alias;
+ ASSERT_STREQ(address, butil::endpoint2str(ep).c_str());
+ ASSERT_EQ(eep, ExtendedEndPoint::address(ep));
+ if (eep != nullptr) {
+ ASSERT_EQ(1, eep->_ref_count.load());
Review Comment:
Asserting the refcount is exactly `1` is brittle: the initial refcount for
an extended endpoint may legitimately differ depending on internal
sharing/caching, and the real regression goal here is that self-assignment does
not change refcount. Consider capturing the refcount before `ep = alias;` and
asserting it’s unchanged after, instead of asserting a fixed value.
--
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]
