acelyc111 commented on code in PR #1843:
URL:
https://github.com/apache/incubator-pegasus/pull/1843#discussion_r1448336415
##########
src/http/http_client.cpp:
##########
@@ -34,6 +34,135 @@ DSN_DEFINE_uint32(http,
"The maximum time in milliseconds that you allow the libcurl
transfer operation "
"to complete");
+#define RETURN_IF_CURL_NOT(expr, expected, ...)
\
+ do {
\
+ const auto code = (expr);
\
+ if (dsn_unlikely(code != expected)) {
\
+ std::string msg(fmt::format("{}: {}", fmt::format(__VA_ARGS__),
to_error_msg(code))); \
+ return dsn::error_s::make(to_error_code(code), msg);
\
+ }
\
+ } while (0)
+
+http_url::http_url() : _url(nullptr) {}
+
+http_url::~http_url() { free_curlu_object(); }
+
+http_url::http_url(http_url &&rhs) : _url(rhs._url) { rhs._url = nullptr; }
+
+http_url &http_url::operator=(http_url &&rhs)
+{
+ _url = rhs._url;
Review Comment:
Prevent self assignment.
if (this != &rhs) {...}
##########
src/http/test/http_client_test.cpp:
##########
@@ -32,10 +33,108 @@
#include "utils/error_code.h"
#include "utils/errors.h"
#include "utils/fmt_logging.h"
+#include "utils/strings.h"
#include "utils/test_macros.h"
namespace dsn {
+using http_url_case =
+ std::tuple<const char *, const char *, uint16_t, const char *, const char
*, const char *>;
Review Comment:
It's too long, how about adding a new struct which contains all the fields,
the struct member names are self-explanatory, and it would be helpful to
simplify the following code.
##########
src/http/http_client.cpp:
##########
@@ -34,6 +34,135 @@ DSN_DEFINE_uint32(http,
"The maximum time in milliseconds that you allow the libcurl
transfer operation "
"to complete");
+#define RETURN_IF_CURL_NOT(expr, expected, ...)
\
+ do {
\
+ const auto code = (expr);
\
+ if (dsn_unlikely(code != expected)) {
\
+ std::string msg(fmt::format("{}: {}", fmt::format(__VA_ARGS__),
to_error_msg(code))); \
Review Comment:
How about swap the places of error code and message, which is the behavior
of other macros like LOG_AND_RETURN_NOT_TRUE, LOG_AND_RETURN_NOT_RDB_OK, etc.
##########
src/http/http_client.cpp:
##########
@@ -34,6 +34,135 @@ DSN_DEFINE_uint32(http,
"The maximum time in milliseconds that you allow the libcurl
transfer operation "
"to complete");
+#define RETURN_IF_CURL_NOT(expr, expected, ...)
\
+ do {
\
+ const auto code = (expr);
\
+ if (dsn_unlikely(code != expected)) {
\
+ std::string msg(fmt::format("{}: {}", fmt::format(__VA_ARGS__),
to_error_msg(code))); \
+ return dsn::error_s::make(to_error_code(code), msg);
\
+ }
\
+ } while (0)
+
+http_url::http_url() : _url(nullptr) {}
+
+http_url::~http_url() { free_curlu_object(); }
+
+http_url::http_url(http_url &&rhs) : _url(rhs._url) { rhs._url = nullptr; }
+
+http_url &http_url::operator=(http_url &&rhs)
+{
+ _url = rhs._url;
Review Comment:
Should free `_url` at first?
Then add a test to check there is no memory leak.
##########
src/http/http_client.cpp:
##########
@@ -34,6 +34,135 @@ DSN_DEFINE_uint32(http,
"The maximum time in milliseconds that you allow the libcurl
transfer operation "
"to complete");
+#define RETURN_IF_CURL_NOT(expr, expected, ...)
\
+ do {
\
+ const auto code = (expr);
\
+ if (dsn_unlikely(code != expected)) {
\
+ std::string msg(fmt::format("{}: {}", fmt::format(__VA_ARGS__),
to_error_msg(code))); \
+ return dsn::error_s::make(to_error_code(code), msg);
\
+ }
\
+ } while (0)
+
+http_url::http_url() : _url(nullptr) {}
+
+http_url::~http_url() { free_curlu_object(); }
+
+http_url::http_url(http_url &&rhs) : _url(rhs._url) { rhs._url = nullptr; }
+
+http_url &http_url::operator=(http_url &&rhs)
+{
+ _url = rhs._url;
+ rhs._url = nullptr;
+ return *this;
+}
+
+dsn::error_s http_url::init()
+{
+ if (_url != nullptr) {
+ free_curlu_object();
+ }
+
+ _url = curl_url();
+ if (_url == nullptr) {
+ return dsn::error_s::make(dsn::ERR_CURL_FAILED, "fail to initialize
curl url");
+ }
Review Comment:
Looks like the macro in
https://github.com/apache/incubator-pegasus/pull/1706/files#diff-4cc4ac5293103b26f79ab90be5de1bf7bbe24a37bbdcda43b2f85bbdd8be360eR39,
how about submit the macro at first? @Samunroyu
--
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]
