empiredan commented on code in PR #1811:
URL:
https://github.com/apache/incubator-pegasus/pull/1811#discussion_r1436421062
##########
src/runtime/test/host_port_test.cpp:
##########
@@ -87,6 +87,16 @@ TEST(host_port_test, operators)
ASSERT_NE(hp, hp2);
ASSERT_FALSE(hp.is_invalid());
ASSERT_TRUE(hp2.is_invalid());
+
+ std::string hp_str = "localhost:8080";
+ host_port hp3;
+ ASSERT_TRUE(hp3.from_string(hp_str));
Review Comment:
```suggestion
ASSERT_TRUE(hp3.is_invalid());
ASSERT_TRUE(hp3.from_string(hp_str));
```
##########
src/runtime/test/host_port_test.cpp:
##########
@@ -87,6 +87,16 @@ TEST(host_port_test, operators)
ASSERT_NE(hp, hp2);
ASSERT_FALSE(hp.is_invalid());
ASSERT_TRUE(hp2.is_invalid());
+
+ std::string hp_str = "localhost:8080";
+ host_port hp3;
+ ASSERT_TRUE(hp3.from_string(hp_str));
+ ASSERT_EQ(hp, hp3);
+
+ host_port hp4;
+ std::string hp_str2 = "pegasus:8080";
+ ASSERT_FALSE(hp4.from_string(hp_str2));
Review Comment:
```suggestion
ASSERT_TRUE(hp4.is_invalid());
ASSERT_FALSE(hp4.from_string(hp_str2));
```
##########
src/runtime/test/host_port_test.cpp:
##########
@@ -87,6 +87,16 @@ TEST(host_port_test, operators)
ASSERT_NE(hp, hp2);
ASSERT_FALSE(hp.is_invalid());
ASSERT_TRUE(hp2.is_invalid());
+
+ std::string hp_str = "localhost:8080";
+ host_port hp3;
+ ASSERT_TRUE(hp3.from_string(hp_str));
+ ASSERT_EQ(hp, hp3);
Review Comment:
```suggestion
ASSERT_EQ(hp, hp3);
ASSERT_FALSE(hp3.is_invalid());
```
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,6 +87,33 @@ host_port::host_port(rpc_address addr)
_type = addr.type();
}
+bool host_port::from_string(const std::string s)
Review Comment:
```suggestion
bool host_port::from_string(const std::string &s)
```
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,6 +87,33 @@ host_port::host_port(rpc_address addr)
_type = addr.type();
}
+bool host_port::from_string(const std::string s)
+{
+ auto pos = s.find_last_of(':');
Review Comment:
```suggestion
const auto pos = s.find_last_of(':');
```
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,6 +87,33 @@ host_port::host_port(rpc_address addr)
_type = addr.type();
}
+bool host_port::from_string(const std::string s)
+{
+ auto pos = s.find_last_of(':');
+ if (pos == std::string::npos) {
+ return false;
+ }
+ _host = s.substr(0, pos);
+ std::string port = s.substr(pos + 1);
+
+ if (!internal::buf2unsigned(port, _port) || _port > UINT16_MAX) {
Review Comment:
Consider add a new function `buf2uint16` ? In principle,
`internal::buf2unsigned` is an internal function and should be used only in
`src/utils/string_conv.h`.
On the other hand, there is no need to check the validity of `_port`, which
has been done in `buf2unsigned`:
```suggestion
if (!buf2uint16(port, _port)) {
```
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,6 +87,33 @@ host_port::host_port(rpc_address addr)
_type = addr.type();
}
+bool host_port::from_string(const std::string s)
+{
+ auto pos = s.find_last_of(':');
+ if (pos == std::string::npos) {
+ return false;
+ }
+ _host = s.substr(0, pos);
+ std::string port = s.substr(pos + 1);
+
+ if (!internal::buf2unsigned(port, _port) || _port > UINT16_MAX) {
+ return false;
+ }
+
+ struct addrinfo hints;
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_INET;
+ hints.ai_socktype = SOCK_STREAM;
+ AddrInfo result;
+ auto err_code = GetAddrInfo(_host, hints, &result);
+ if (dsn_unlikely(!err_code.is_ok())) {
Review Comment:
```suggestion
if (dsn_unlikely(!GetAddrInfo(_host, hints, &result))) {
```
--
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: dev-unsubscr...@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@pegasus.apache.org
For additional commands, e-mail: dev-h...@pegasus.apache.org
