lordgamez commented on code in PR #2029:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2029#discussion_r2369375348
##########
core-framework/src/utils/SystemCpuUsageTracker.cpp:
##########
@@ -76,7 +76,7 @@ double
SystemCpuUsageTracker::getCpuUsageBetweenLastTwoQueries() const {
if (total_diff + total_idle_diff == 0) {
return -1.0;
}
- double percent = static_cast<double>(total_diff) /
static_cast<double>(total_diff + total_idle_diff);
+ double percent = static_cast<double>(total_diff) /
static_cast<double>(total_diff + total_idle_diff); //
NOLINT(clang-analyzer-optin.taint.TaintedDiv)
Review Comment:
When clang tidy was introduced I remember that it found cases where 0
division was a possible issue, so I wouldn't disable it for 1 false positive if
it can help in other cases.
##########
extensions/kafka/PublishKafka.cpp:
##########
@@ -112,7 +112,7 @@ class PublishKafka::Messages {
}
template<typename Func>
- auto modifyResult(size_t index, Func fun) ->
decltype(fun(flow_files_.at(index))) {
+ auto modifyResult(size_t index, const Func& fun) ->
decltype(fun(flow_files_.at(index))) {
Review Comment:
Clang tidy warns for every unnecessary copy and suggests const ref instead.
##########
.clang-tidy:
##########
@@ -1,6 +1,7 @@
Checks: >
-clang-analyzer-optin.cplusplus.VirtualCall,
-clang-analyzer-optin.core.EnumCastOutOfRange,
+ -clang-analyzer-unix.BlockInCriticalSection,
Review Comment:
As far as I remember there was an issue with a system call in a loop which
did make sense in the context, and I think we should be able to have more
judgement around these performance issues than the checker, so I didn't find
this useful.
##########
extensions/sftp/processors/ListSFTP.cpp:
##########
@@ -690,7 +690,7 @@ void ListSFTP::listByTrackingEntities(
time_t now = time(nullptr);
uint64_t min_timestamp_to_list = (!initial_listing_complete_ &&
entity_tracking_initial_listing_target_ ==
ENTITY_TRACKING_INITIAL_LISTING_TARGET_ALL_AVAILABLE)
- ? 0U : (now * 1000 - entity_tracking_time_window.count());
+ ? 0U : ((now * 1000) - entity_tracking_time_window.count());
Review Comment:
We should decide if we want to follow this check at all and explicitly put
all parentheses in expressions or just disable this check altogether. I suppose
it is better to judge ourselves which expression is complex enough to require
explicit parentheses so we can disable this check.
##########
extensions/standard-processors/utils/JoltUtils.h:
##########
@@ -58,7 +58,7 @@ class Spec {
return nullptr;
}
- template<std::invocable<std::shared_ptr<core::logging::Logger>> OnEnterFn,
std::invocable<std::shared_ptr<core::logging::Logger>> OnExitFn>
+ template<std::invocable<const std::shared_ptr<core::logging::Logger>&>
OnEnterFn, std::invocable<const std::shared_ptr<core::logging::Logger>&>
OnExitFn>
Review Comment:
This was again the check that suggests using cont refs instead of copying
for the lambda parameters. I think it's better in this case to avoid increasing
the shared pointer ref counter when it's unnecessary.
##########
extensions/systemd/ConsumeJournald.cpp:
##########
@@ -78,7 +78,7 @@ void ConsumeJournald::onSchedule(core::ProcessContext&
context, core::ProcessSes
return process_old_messages ? journal.seekHead() : journal.seekTail();
};
worker_->enqueue([this, &seek_default] {
- const auto cursor = state_manager_->get() |
utils::transform([](std::unordered_map<std::string, std::string>&& m) { return
m.at(CURSOR_KEY); });
+ const auto cursor = state_manager_->get() | utils::transform([](const
std::unordered_map<std::string, std::string>& m) { return m.at(CURSOR_KEY); });
Review Comment:
clang tidy warns for using rvalue references for parameters that are not
moved in the function body
--
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]
