fgerlits commented on code in PR #1670:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1670#discussion_r1348487073
##########
libminifi/src/core/flow/StructuredConfiguration.cpp:
##########
@@ -235,20 +235,20 @@ void StructuredConfiguration::parseProcessorNode(const
Node& processors_node, co
if (procCfg.schedulingStrategy == "TIMER_DRIVEN" ||
procCfg.schedulingStrategy == "EVENT_DRIVEN") {
if (auto scheduling_period =
utils::timeutils::StringToDuration<std::chrono::nanoseconds>(procCfg.schedulingPeriod))
{
- logger_->log_debug("convert: parseProcessorNode: schedulingPeriod =>
[%" PRId64 "] ns", scheduling_period->count());
+ logger_->log_debug("convert: parseProcessorNode: schedulingPeriod =>
[{}] ns", scheduling_period->count());
Review Comment:
the ` ns` and the `count()` call could be removed here, too
##########
libminifi/src/c2/ControllerSocketProtocol.cpp:
##########
@@ -401,7 +401,7 @@ asio::awaitable<void>
ControllerSocketProtocol::handleCommand(std::unique_ptr<io
handleDescribe(*stream);
break;
default:
- logger_->log_error("Unhandled C2 operation: %s", std::to_string(head));
+ logger_->log_error("Unhandled C2 operation: {}", std::to_string(head));
Review Comment:
very unimportant, but the `std::to_string()` is no longer needed
##########
libminifi/src/core/state/LogMetricsPublisher.cpp:
##########
@@ -59,10 +59,10 @@ void LogMetricsPublisher::readLoggingInterval() {
if (auto logging_interval_str =
configuration_->get(Configure::nifi_metrics_publisher_log_metrics_logging_interval))
{
if (auto logging_interval =
minifi::core::TimePeriodValue::fromString(logging_interval_str.value())) {
logging_interval_ = logging_interval->getMilliseconds();
- logger_->log_info("Metric logging interval is set to %" PRId64 "
milliseconds", int64_t{logging_interval_.count()});
+ logger_->log_info("Metric logging interval is set to {} milliseconds",
int64_t{logging_interval_.count()});
Review Comment:
this could also be
```suggestion
logger_->log_info("Metric logging interval is set to {}",
logging_interval_);
```
##########
libminifi/include/core/logging/Logger.h:
##########
@@ -112,19 +62,42 @@ enum LOG_LEVEL {
off = 6
};
+inline spdlog::level::level_enum mapToSpdLogLevel(LOG_LEVEL level) {
+ switch (level) {
+ case trace: return spdlog::level::trace;
+ case debug: return spdlog::level::debug;
+ case info: return spdlog::level::info;
+ case warn: return spdlog::level::warn;
+ case err: return spdlog::level::err;
+ case critical: return spdlog::level::critical;
+ case off: return spdlog::level::off;
+ }
+ throw std::invalid_argument(fmt::format("Invalid LOG_LEVEL {}",
magic_enum::enum_underlying(level)));
+}
+
+inline LOG_LEVEL mapFromSpdLogLevel(spdlog::level::level_enum level) {
+ switch (level) {
+ case spdlog::level::trace: return LOG_LEVEL::trace;
+ case spdlog::level::debug: return LOG_LEVEL::debug;
+ case spdlog::level::info: return LOG_LEVEL::info;
+ case spdlog::level::warn: return LOG_LEVEL::warn;
+ case spdlog::level::err: return LOG_LEVEL::err;
+ case spdlog::level::critical: return LOG_LEVEL::critical;
+ case spdlog::level::off: return LOG_LEVEL::off;
+ case spdlog::level::n_levels: return LOG_LEVEL::off;
Review Comment:
`n_levels` is just a technical enum value to make it easier to count the
real enum values; I would treat it as any other invalid argument, and throw an
exception in this case (eg. fall through)
##########
libminifi/include/core/logging/Logger.h:
##########
@@ -112,19 +62,42 @@ enum LOG_LEVEL {
off = 6
};
+inline spdlog::level::level_enum mapToSpdLogLevel(LOG_LEVEL level) {
+ switch (level) {
+ case trace: return spdlog::level::trace;
+ case debug: return spdlog::level::debug;
+ case info: return spdlog::level::info;
+ case warn: return spdlog::level::warn;
+ case err: return spdlog::level::err;
+ case critical: return spdlog::level::critical;
+ case off: return spdlog::level::off;
+ }
+ throw std::invalid_argument(fmt::format("Invalid LOG_LEVEL {}",
magic_enum::enum_underlying(level)));
+}
+
+inline LOG_LEVEL mapFromSpdLogLevel(spdlog::level::level_enum level) {
+ switch (level) {
+ case spdlog::level::trace: return LOG_LEVEL::trace;
+ case spdlog::level::debug: return LOG_LEVEL::debug;
+ case spdlog::level::info: return LOG_LEVEL::info;
+ case spdlog::level::warn: return LOG_LEVEL::warn;
+ case spdlog::level::err: return LOG_LEVEL::err;
+ case spdlog::level::critical: return LOG_LEVEL::critical;
+ case spdlog::level::off: return LOG_LEVEL::off;
+ case spdlog::level::n_levels: return LOG_LEVEL::off;
+ }
+ throw std::invalid_argument(fmt::format("Invalid spdlog::level::level_enum
{}", magic_enum::enum_underlying(level)));
+}
+
class BaseLogger {
public:
virtual ~BaseLogger();
virtual void log_string(LOG_LEVEL level, std::string str) = 0;
-
- virtual bool should_log(const LOG_LEVEL &level);
+ virtual bool should_log(LOG_LEVEL level) = 0;
+ [[nodiscard]] virtual LOG_LEVEL level() const = 0;
};
-/**
- * LogBuilder is a class to facilitate using the LOG macros below and an
associated put-to operator.
- *
- */
class LogBuilder {
Review Comment:
do we use this `LogBuilder` anywhere?
##########
libminifi/src/core/logging/LoggerConfiguration.cpp:
##########
@@ -305,7 +305,7 @@ std::shared_ptr<spdlog::logger>
LoggerConfiguration::get_logger(const std::share
}
if (logger != nullptr) {
const auto levelView(spdlog::level::to_string_view(level));
- logger->log_debug("%s logger got sinks from namespace %s and level %s from
namespace %s", name, sink_namespace_str, std::string(levelView.begin(),
levelView.end()), level_namespace_str);
+ logger->log_debug("{} logger got sinks from namespace {} and level {} from
namespace {}", name, sink_namespace_str, std::string(levelView.begin(),
levelView.end()), level_namespace_str);
Review Comment:
this could be simplified to
```c++
logger->log_debug("{} logger got sinks from namespace {} and level {}
from namespace {}", name, sink_namespace_str,
spdlog::level::to_string_view(level), level_namespace_str);
```
##########
libminifi/src/utils/net/AsioSocketUtils.cpp:
##########
@@ -72,18 +72,18 @@ bool AsioSocketConnection::connectTcpSocketOverSsl() {
asio::error_code err;
asio::ip::tcp::resolver::results_type endpoints =
resolver.resolve(socket_data_.host, std::to_string(socket_data_.port), err);
if (err) {
- logger_->log_error("Resolving host '%s' on port '%s' failed with the
following message: '%s'", socket_data_.host, std::to_string(socket_data_.port),
err.message());
+ logger_->log_error("Resolving host '{}' on port '{}' failed with the
following message: '{}'", socket_data_.host, std::to_string(socket_data_.port),
err.message());
Review Comment:
all these `std::to_string()`s in this file could be removed
##########
libminifi/test/TestBase.cpp:
##########
@@ -59,7 +59,7 @@ std::shared_ptr<LogTestController>
LogTestController::getInstance(const std::sha
void LogTestController::setLevel(std::string_view name,
spdlog::level::level_enum level) {
const auto levelView(spdlog::level::to_string_view(level));
- logger_->log_info("Setting log level for %s to %s", std::string(name),
std::string(levelView.begin(), levelView.end()));
+ logger_->log_info("Setting log level for {} to {}", std::string(name),
std::string(levelView.begin(), levelView.end()));
Review Comment:
could be simplified:
```c++
logger_->log_info("Setting log level for {} to {}", name,
spdlog::level::to_string_view(level));
```
--
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]
