szaszm commented on a change in pull request #1267:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1267#discussion_r814385509
##########
File path: extensions/http-curl/protocols/RESTSender.cpp
##########
@@ -161,6 +161,7 @@ C2Payload RESTSender::sendPayload(const std::string url,
const Direction directi
logger_->log_debug("Response code '" "%" PRId64 "' from '%s'", respCode,
url);
}
const auto response_body_bytes =
gsl::make_span(client.getResponseBody()).as_span<const std::byte>();
+ logger_->log_trace("Received response: \"%s\"", [&] {return
utils::StringUtils::escapeUnprintableBytes(response_body_bytes);});
Review comment:
I think extending `HeartbeatReporter` and `HeartbeatLogger` would be a
better approach, because it's consistent with what we're doing for heartbeats.
One can consider the response to be related to the heartbeat, so I think it's
fine to just extend that interface/implementation pair with a C2 response
observer callback.
##########
File path: libminifi/include/core/logging/Logger.h
##########
@@ -53,28 +54,32 @@ class LoggerControl {
std::atomic<bool> is_enabled_;
};
-inline char const* conditional_conversion(std::string const& str) {
- return str.c_str();
-}
+template<typename Arg>
+using has_const_c_str_method = decltype(std::declval<const Arg&>().c_str());
-template<size_t N>
-inline char const* conditional_conversion(const utils::SmallString<N>& arr) {
- return arr.c_str();
+template<typename Arg>
+inline decltype(auto) conditional_stringify(Arg&& arg) {
+ if constexpr (utils::meta::is_detected_v<has_const_c_str_method, Arg> ||
std::is_scalar_v<std::decay_t<Arg>>) {
+ return std::forward<Arg>(arg);
+ } else if constexpr (std::is_invocable_v<Arg>) {
+ return std::forward<Arg>(arg)();
Review comment:
Use
[`std::invoke`](https://en.cppreference.com/w/cpp/utility/functional/invoke) to
avoid errors with member pointers.
[`std::is_invocable_v`](https://en.cppreference.com/w/cpp/types/is_invocable)
checks that the argument is
[`Callable`](https://en.cppreference.com/w/cpp/named_req/Callable) (can be used
with `std::invoke`) with the specified arguments, but `Callable` is more
general than
[`FunctionObject`](https://en.cppreference.com/w/cpp/named_req/FunctionObject)
(can be used with the function call operator).
##########
File path: libminifi/include/core/logging/Logger.h
##########
@@ -53,28 +54,32 @@ class LoggerControl {
std::atomic<bool> is_enabled_;
};
-inline char const* conditional_conversion(std::string const& str) {
- return str.c_str();
-}
+template<typename Arg>
+using has_const_c_str_method = decltype(std::declval<const Arg&>().c_str());
-template<size_t N>
-inline char const* conditional_conversion(const utils::SmallString<N>& arr) {
- return arr.c_str();
+template<typename Arg>
+inline decltype(auto) conditional_stringify(Arg&& arg) {
Review comment:
Not an explicit issue, but I just wanted to express my general
discomfort about `decltype(auto)`. Its usage in this PR seems to be fine, but
generally I prefer to avoid it if possible because there are some surprising
edge cases.
##########
File path: libminifi/src/c2/C2Agent.cpp
##########
@@ -243,7 +243,7 @@ void C2Agent::performHeartBeat() {
payload.addPayload(std::move(child_metric_payload));
}
}
- C2Payload && response = protocol_.load()->consumePayload(payload);
+ C2Payload&& response = protocol_.load()->consumePayload(payload);
Review comment:
If you change this line, please remove the reference qualifier. It only
works because of reference lifetime extension, and we shouldn't rely on it here.
--
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]
