szaszm commented on code in PR #2029:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2029#discussion_r2368523396
##########
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:
disagree with the extra parens: multiplication has higher precedence, it's
not confusing, and it's on the left side anyway, so it's the same even in left
to right order.
##########
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:
why was this changed? we can capture a temporary by either reference type,
but it's an rvalue, so an rvalue reference seems better fitting. It would allow
out the cursor value if unordered_map had an rvalue qualified overload of at.
##########
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:
why was this changed? if there is no good justification, I'd change it back.
##########
extensions/standard-processors/processors/SplitText.cpp:
##########
@@ -53,7 +53,7 @@ uint8_t LineReader::getEndLineSize(size_t newline_position) {
}
void LineReader::setLastLineInfoAttributes(uint8_t endline_size, const
std::optional<std::string>& starts_with) {
- const uint64_t size_from_beginning_of_stream = (current_buffer_count_ - 1) *
buffer_size_ + buffer_offset_;
+ const uint64_t size_from_beginning_of_stream = ((current_buffer_count_ - 1)
* buffer_size_) + buffer_offset_;
Review Comment:
same thing about parens here
##########
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:
disagree with this change: Whether the argument is const-ref or not doesn't
make a difference from the standpoint of being able to call a function with it
or not.
##########
libminifi/test/unit/ResponseNodeLoaderTests.cpp:
##########
@@ -49,7 +50,7 @@ class ResponseNodeLoaderTestFixture {
}
protected:
- template<typename T, typename = typename
std::enable_if_t<std::is_base_of_v<minifi::core::ProcessorImpl, T>>>
+ template<typename T> requires(std::is_base_of_v<minifi::core::ProcessorImpl,
T>)
Review Comment:
unnecessary parens
##########
extensions/standard-processors/processors/AttributeRollingWindow.cpp:
##########
@@ -113,7 +113,7 @@ void
AttributeRollingWindow::calculateAndSetAttributes(core::FlowFile &flow_file
}());
//
https://math.stackexchange.com/questions/1720876/sums-of-squares-minus-square-of-sums
const auto avg_of_squares = std::accumulate(std::begin(sorted_values),
std::end(sorted_values), 0.0, [&](double acc, double value) {
- return acc + std::pow(value, 2) /
gsl::narrow_cast<double>(sorted_values.size());
+ return acc + (std::pow(value, 2) /
gsl::narrow_cast<double>(sorted_values.size()));
Review Comment:
once again I disagree with the parens: division has higher precedence than
addition, and I don't think anyone finds that confusing. same as in math
##########
.clang-tidy:
##########
@@ -1,6 +1,7 @@
Checks: >
-clang-analyzer-optin.cplusplus.VirtualCall,
-clang-analyzer-optin.core.EnumCastOutOfRange,
+ -clang-analyzer-unix.BlockInCriticalSection,
Review Comment:
why was this disabled?
##########
core-framework/src/utils/SystemCpuUsageTracker.cpp:
##########
@@ -44,7 +44,7 @@ void SystemCpuUsageTracker::queryCpuTimes() {
previous_total_sys_ = total_sys_;
previous_total_idle_ = total_idle_;
gsl::owner<FILE*> file = fopen("/proc/stat", "r");
- if (fscanf(file, "cpu %lu %lu %lu %lu", &total_user_, &total_user_low_,
&total_sys_, &total_idle_) != 4) { // NOLINT(cert-err34-c)
+ if (fscanf(file, "cpu %lu %lu %lu %lu", &total_user_, &total_user_low_,
&total_sys_, &total_idle_) != 4) { //
NOLINT(cert-err34-c,clang-analyzer-unix.Stream)
Review Comment:
This warning looks valid to me. We should handle the NULL case.
```suggestion
if (!file) { return; }
if (fscanf(file, "cpu %lu %lu %lu %lu", &total_user_, &total_user_low_,
&total_sys_, &total_idle_) != 4) { // NOLINT(cert-err34-c)
```
##########
extensions/standard-processors/tests/unit/SplitTextTests.cpp:
##########
@@ -81,7 +81,7 @@ TEST_CASE("Test LineReader with input larger than buffer
length") {
TEST_CASE("Test LineReader with input of same size as buffer length") {
auto stream = std::make_shared<io::BufferStream>();
- std::string input = std::string(BUFFER_SIZE - 1, 'a') + "\n" +
std::string(BUFFER_SIZE * 2 - 1, 'b') + "\n";
+ std::string input = std::string(BUFFER_SIZE - 1, 'a') + "\n" +
std::string((BUFFER_SIZE * 2) - 1, 'b') + "\n";
Review Comment:
same thing about parens here
##########
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:
I'd rather disable this check globally. If it can't prove from the previous
check that the denominator is not zero, then it's mostly useless and not worth
the overhead of maintaining NOLINT comments.
--
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]
