szaszm commented on code in PR #1670:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1670#discussion_r1340005252
##########
extensions/windows-event-log/ConsumeWindowsEventLog.cpp:
##########
@@ -628,7 +628,7 @@ void ConsumeWindowsEventLog::LogWindowsError(const
std::string& error) const {
(LPTSTR)&lpMsg,
0, nullptr);
- logger_->log_error((error + " %x: %s\n").c_str(),
static_cast<int>(error_id), reinterpret_cast<char *>(lpMsg));
+ logger_->log_error((error + " {:#x}: {}\n").c_str(),
static_cast<int>(error_id), reinterpret_cast<char *>(lpMsg));
Review Comment:
Does this compile? The format string argument needs to be a compile time
string literal. Also, if we're touching this line, I'd remove the '\n' from the
end, this is a log message after all.
##########
libminifi/include/core/logging/Logger.h:
##########
@@ -49,59 +50,15 @@ class LoggerControl {
std::atomic<bool> is_enabled_;
};
-template<typename Arg>
-using has_const_c_str_method = decltype(std::declval<const Arg&>().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>) {
+ if constexpr (std::is_invocable_v<Arg>) {
Review Comment:
`invocable` corresponds to `invoke`, not the function call operator.
Is the function still used btw? If it is, it does something wildly different
now, so it should be renamed appropriately.
##########
libminifi/include/core/logging/Logger.h:
##########
@@ -234,33 +196,46 @@ class Logger : public BaseLogger {
std::mutex mutex_;
private:
- template<typename ...Args>
- inline void log(spdlog::level::level_enum level, const char* const format,
Args&& ...args) {
+ std::string trimToMaxSizeAndAddId(std::string&& my_string) {
+ auto max_log_size = max_log_size_.load();
+ if (max_log_size >= 0 && my_string.size() >
gsl::narrow<size_t>(max_log_size))
+ my_string = my_string.substr(0, max_log_size);
+ if (auto id = get_id()) {
+ my_string += *id;
+ }
+ return my_string;
+ }
+
+ template<typename ...T>
+ std::string stringify(fmt::format_string<T...> fmt, T&&... args) {
+ auto log_message = fmt::format(fmt, std::forward<T>(args)...);
+ return trimToMaxSizeAndAddId(std::move(log_message));
+ }
+
+ template<typename ...T>
+ inline void log(spdlog::level::level_enum level,
fmt::format_string<std::invoke_result_t<decltype(map_args), T>...> fmt, T&&
...args) {
if (controller_ && !controller_->is_enabled())
- return;
+ return;
std::lock_guard<std::mutex> lock(mutex_);
if (!delegate_->should_log(level)) {
return;
}
- auto str = format_string(max_log_size_.load(), format,
conditional_stringify(std::forward<Args>(args))...);
- if (const auto id = get_id()) {
- str = str + *id;
- }
- delegate_->log(level, str);
+ delegate_->log(level, stringify(fmt, map_args(std::forward<T>(args))...));
}
std::atomic<int> max_log_size_{LOG_BUFFER_SIZE};
};
+#define LOG_TRACE(x) LogBuilder((x).get(),
org::apache::nifi::minifi::core::logging::LOG_LEVEL::trace)
Review Comment:
Can we remove these macros?
##########
libminifi/src/sitetosite/RawSocketProtocol.cpp:
##########
@@ -435,8 +431,8 @@ int RawSiteToSiteClient::readRespond(const
std::shared_ptr<Transaction> &transac
return readResponse(transaction, code, message);
}
-int RawSiteToSiteClient::writeRespond(const std::shared_ptr<Transaction>
&transaction, RespondCode code, std::string message) {
- return writeResponse(transaction, code, std::move(message));
+int RawSiteToSiteClient::writeRespond(const std::shared_ptr<Transaction>
&transaction, RespondCode code, const std::string& message) {
+ return writeResponse(transaction, code, message);
Review Comment:
What about changing to `std::string_view` instead?
##########
libminifi/include/core/logging/Logger.h:
##########
@@ -145,81 +125,63 @@ class LogBuilder {
bool ignore;
BaseLogger *ptr;
std::stringstream str;
- LOG_LEVEL level;
+ LOG_LEVEL level_;
+};
+
+template<class... Ts> struct overloaded : Ts... { using Ts::operator()...; };
+template<class... Ts> overloaded(Ts...) -> overloaded<Ts...>;
Review Comment:
This belongs to `GeneralUtils.h` if it's not already there IMO. It may work
even without the deduction guide with out target compilers.
##########
libminifi/include/core/logging/Logger.h:
##########
@@ -145,81 +125,63 @@ class LogBuilder {
bool ignore;
BaseLogger *ptr;
std::stringstream str;
- LOG_LEVEL level;
+ LOG_LEVEL level_;
Review Comment:
It's all public, so this is basically a simple struct. The underscore ending
for members don't apply to these types. And the other data members don't follow
this either.
```suggestion
LOG_LEVEL level;
```
##########
libminifi/include/utils/ResourceQueue.h:
##########
@@ -108,16 +108,16 @@ class ResourceQueue : public
std::enable_shared_from_this<ResourceQueue<Resource
private:
void returnResource(std::unique_ptr<ResourceType> resource) {
- logDebug("Returning [%p] resource", resource.get());
+ logDebug("Returning [{}] resource", static_cast<void*>(resource.get()));
if (reset_fn_)
reset_fn_.value()(*resource);
internal_queue_.enqueue(std::move(resource));
}
template<typename ...Args>
- void logDebug(const char * const format, Args&& ...args) {
+ void
logDebug(fmt::format_string<std::invoke_result_t<decltype(core::logging::map_args),
Args>...> fmt, Args&& ...args) {
if (logger_)
- logger_->log_debug(format, std::forward<Args>(args)...);
+ logger_->log_debug(fmt, std::forward<Args>(args)...);
Review Comment:
extra space
##########
libminifi/include/utils/ResourceQueue.h:
##########
@@ -108,16 +108,16 @@ class ResourceQueue : public
std::enable_shared_from_this<ResourceQueue<Resource
private:
void returnResource(std::unique_ptr<ResourceType> resource) {
- logDebug("Returning [%p] resource", resource.get());
+ logDebug("Returning [{}] resource", static_cast<void*>(resource.get()));
if (reset_fn_)
reset_fn_.value()(*resource);
internal_queue_.enqueue(std::move(resource));
}
template<typename ...Args>
- void logDebug(const char * const format, Args&& ...args) {
+ void
logDebug(fmt::format_string<std::invoke_result_t<decltype(core::logging::map_args),
Args>...> fmt, Args&& ...args) {
Review Comment:
If this is going to be part of the public interface of `Logger`, then I'd
add a public type alias for this metaprogramming magic, so users don't have to
decipher all of this.
--
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]
