You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "lordgamez (via GitHub)" <gi...@apache.org> on 2023/09/28 12:11:17 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request, #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
lordgamez opened a new pull request, #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671
Depends on https://github.com/apache/nifi-minifi-cpp/pull/1661
https://issues.apache.org/jira/browse/MINIFICPP-1891
----------------
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1340352435
##########
extensions/expression-language/tests/ExpressionLanguageTests.cpp:
##########
@@ -1321,13 +1321,13 @@ TEST_CASE("Now Date", "[expressionNowDate]") {
TEST_CASE("Parse RFC3339 with Expression Language toDate") {
using date::sys_days;
using org::apache::nifi::minifi::utils::timeutils::parseRfc3339;
- using namespace date::literals;
+ using namespace date::literals; // NOLINT(google-build-using-namespace)
using namespace std::literals::chrono_literals;
using std::chrono::milliseconds;
- milliseconds expected_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s).time_since_epoch());
- milliseconds expected_tenth_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s + 100ms).time_since_epoch());
- milliseconds expected_milli_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s + 190ms).time_since_epoch());
+ milliseconds expected_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s).time_since_epoch()); // NOLINT(whitespace/braces)
+ milliseconds expected_tenth_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s + 100ms).time_since_epoch()); // NOLINT(whitespace/braces)
+ milliseconds expected_milli_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s + 190ms).time_since_epoch()); // NOLINT(whitespace/braces)
Review Comment:
What was the error here?
##########
extensions/sftp/client/SFTPClient.cpp:
##########
@@ -83,7 +83,7 @@ static const char* sftp_strerror(unsigned long err) { // NOLINT(runtime/int) un
}
}
-static SFTPError libssh2_sftp_error_to_sftp_error(unsigned long libssh2_sftp_error) { // NOLINT(runtime/int) unsigned long comes from libssh2 API
+static SFTPError libssh2_sftp_error_to_sftp_error(unsigned long libssh2_sftp_error) { // NOLINT(runtime/int,google-runtime-int) unsigned long comes from libssh2 API
Review Comment:
We can disable one of these, so we only need one NOLINT for the same rule
##########
extensions/systemd/tests/ConsumeJournaldTest.cpp:
##########
@@ -90,7 +90,7 @@ struct TestJournal final : libwrapper::Journal {
}
int getCursor(gsl::owner<char*>* const cursor_out) noexcept override {
- *cursor_out = gsl::owner<char*>(strdup(std::to_string(consumed).c_str()));
+ *cursor_out = strdup(std::to_string(consumed).c_str()); // NOLINT(cppcoreguidelines-owning-memory)
Review Comment:
Why remove the `gsl::owner` annotation?
##########
extensions/systemd/libwrapper/DlopenWrapper.cpp:
##########
@@ -72,7 +72,7 @@ class DlopenJournal : public Journal {
template<typename F>
F loadSymbol(const char* const symbol_name) {
// The cast below is supported by POSIX platforms. https://stackoverflow.com/a/1096349
- F const symbol = (F)dlsym(libhandle_.get(), symbol_name);
+ F const symbol = reinterpret_cast<F>(dlsym(libhandle_.get(), symbol_name));
Review Comment:
A cast from object to function pointers is normally undefined behavior, but in the case of dlsym, this is allowed on POSIX. I'm not sure that a `reinterpret_cast` is guaranteed to do the right thing in this case. I'd rather keep the C-style cast here, because that's guaranteed to work, while with `reinterpret_cast`, an implementation may decide later that it's undefined behavior to do this cast even with `dlsym`.
##########
extensions/standard-processors/processors/PutTCP.cpp:
##########
@@ -215,7 +215,7 @@ asio::awaitable<std::error_code> ConnectionHandler<SocketType>::establishNewConn
continue;
}
if (max_size_of_socket_send_buffer_)
- socket.lowest_layer().set_option(TcpSocket::send_buffer_size(*max_size_of_socket_send_buffer_));
+ socket.lowest_layer().set_option(TcpSocket::send_buffer_size(*max_size_of_socket_send_buffer_)); // NOLINT(google-readability-casting)
Review Comment:
What was the problem 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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-1891 Integrate google clang-tidy checks in CI [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1346068822
##########
extensions/librdkafka/ConsumeKafka.cpp:
##########
@@ -230,7 +230,8 @@ std::vector<std::unique_ptr<rd_kafka_message_t, utils::rd_kafka_message_deleter>
while (messages.size() < max_poll_records_ && elapsed < max_poll_time_milliseconds_) {
logger_->log_debug("Polling for new messages for %d milliseconds...", max_poll_time_milliseconds_.count());
std::unique_ptr<rd_kafka_message_t, utils::rd_kafka_message_deleter>
- message { rd_kafka_consumer_poll(consumer_.get(), std::chrono::duration_cast<std::chrono::milliseconds>(max_poll_time_milliseconds_ - elapsed).count()), utils::rd_kafka_message_deleter() };
+ message { rd_kafka_consumer_poll(consumer_.get(), gsl::narrow<int>(std::chrono::duration_cast<std::chrono::milliseconds>(max_poll_time_milliseconds_ - elapsed).count())),
+ utils::rd_kafka_message_deleter() };
Review Comment:
```suggestion
message { rd_kafka_consumer_poll(consumer_.get(), gsl::narrow<int>(std::chrono::duration_cast<std::chrono::milliseconds>(max_poll_time_milliseconds_ - elapsed).count())) };
```
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1341420013
##########
extensions/expression-language/tests/ExpressionLanguageTests.cpp:
##########
@@ -1321,13 +1321,13 @@ TEST_CASE("Now Date", "[expressionNowDate]") {
TEST_CASE("Parse RFC3339 with Expression Language toDate") {
using date::sys_days;
using org::apache::nifi::minifi::utils::timeutils::parseRfc3339;
- using namespace date::literals;
+ using namespace date::literals; // NOLINT(google-build-using-namespace)
using namespace std::literals::chrono_literals;
using std::chrono::milliseconds;
- milliseconds expected_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s).time_since_epoch());
- milliseconds expected_tenth_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s + 100ms).time_since_epoch());
- milliseconds expected_milli_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s + 190ms).time_since_epoch());
+ milliseconds expected_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s).time_since_epoch()); // NOLINT(whitespace/braces)
+ milliseconds expected_tenth_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s + 100ms).time_since_epoch()); // NOLINT(whitespace/braces)
+ milliseconds expected_milli_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s + 190ms).time_since_epoch()); // NOLINT(whitespace/braces)
Review Comment:
And what necessitated `// NOLINT(whitespace/braces)`?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1341463466
##########
extensions/systemd/tests/ConsumeJournaldTest.cpp:
##########
@@ -90,7 +90,7 @@ struct TestJournal final : libwrapper::Journal {
}
int getCursor(gsl::owner<char*>* const cursor_out) noexcept override {
- *cursor_out = gsl::owner<char*>(strdup(std::to_string(consumed).c_str()));
+ *cursor_out = strdup(std::to_string(consumed).c_str()); // NOLINT(cppcoreguidelines-owning-memory)
Review Comment:
I was referring to the example assigning the returned value of a function to a `gsl::owner<>` type, as we are assigning to the pointed value I don't think the pointer parameter is relevant in this case, the warning would be the same if we only had a `gsl::owner<char*>` type parameter. I can revert it as the C-style cast check will be removed.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1341466297
##########
extensions/expression-language/tests/ExpressionLanguageTests.cpp:
##########
@@ -1321,13 +1321,13 @@ TEST_CASE("Now Date", "[expressionNowDate]") {
TEST_CASE("Parse RFC3339 with Expression Language toDate") {
using date::sys_days;
using org::apache::nifi::minifi::utils::timeutils::parseRfc3339;
- using namespace date::literals;
+ using namespace date::literals; // NOLINT(google-build-using-namespace)
using namespace std::literals::chrono_literals;
using std::chrono::milliseconds;
- milliseconds expected_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s).time_since_epoch());
- milliseconds expected_tenth_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s + 100ms).time_since_epoch());
- milliseconds expected_milli_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s + 190ms).time_since_epoch());
+ milliseconds expected_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s).time_since_epoch()); // NOLINT(whitespace/braces)
+ milliseconds expected_tenth_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s + 100ms).time_since_epoch()); // NOLINT(whitespace/braces)
+ milliseconds expected_milli_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s + 190ms).time_since_epoch()); // NOLINT(whitespace/braces)
Review Comment:
That's also a false positive as it wants to add a space after the brace in `sys_days{`
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-1891 Integrate google clang-tidy checks in CI [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1371278925
##########
extensions/sftp/tests/PutSFTPTests.cpp:
##########
@@ -201,7 +201,7 @@ class PutSFTPTestsFixture {
namespace {
std::size_t directoryContentCount(const std::filesystem::path& dir) {
- return (std::size_t)std::distance(std::filesystem::directory_iterator{dir}, std::filesystem::directory_iterator{});
+ return static_cast<std::size_t>(std::distance(std::filesystem::directory_iterator{dir}, std::filesystem::directory_iterator{}));
Review Comment:
Updated in fdbd40841b88732109b084214e3cb027492d4bac
##########
libminifi/src/core/ProcessSession.cpp:
##########
@@ -175,7 +175,7 @@ std::shared_ptr<core::FlowFile> ProcessSession::cloneDuringTransfer(const std::s
}
std::shared_ptr<core::FlowFile> ProcessSession::clone(const std::shared_ptr<core::FlowFile> &parent, int64_t offset, int64_t size) {
- if ((uint64_t) (offset + size) > parent->getSize()) {
+ if (static_cast<uint64_t>(offset + size) > parent->getSize()) {
Review Comment:
Updated in fdbd40841b88732109b084214e3cb027492d4bac
##########
libminifi/src/core/flow/StructuredConfiguration.cpp:
##########
@@ -266,10 +266,10 @@ void StructuredConfiguration::parseProcessorNode(const Node& processors_node, co
logger_->log_debug("setting scheduling strategy as {}", procCfg.schedulingStrategy);
}
- int32_t maxConcurrentTasks;
+ int32_t maxConcurrentTasks = 0;
if (core::Property::StringToInt(procCfg.maxConcurrentTasks, maxConcurrentTasks)) {
logger_->log_debug("parseProcessorNode: maxConcurrentTasks => [{}]", maxConcurrentTasks);
- processor->setMaxConcurrentTasks((uint8_t) maxConcurrentTasks);
+ processor->setMaxConcurrentTasks(static_cast<uint8_t>(maxConcurrentTasks));
}
Review Comment:
Yes, good idea, updated in fdbd40841b88732109b084214e3cb027492d4bac
##########
libminifi/src/provenance/Provenance.cpp:
##########
@@ -249,17 +249,17 @@ bool ProvenanceEventRecord::deserialize(io::InputStream &input_stream) {
}
}
- uint32_t eventType;
+ uint32_t eventType = 0;
{
const auto ret = input_stream.read(eventType);
if (ret != 4) {
return false;
}
}
- this->_eventType = (ProvenanceEventRecord::ProvenanceEventType) eventType;
+ _eventType = static_cast<ProvenanceEventRecord::ProvenanceEventType>(eventType);
Review Comment:
Good point, updated in fdbd40841b88732109b084214e3cb027492d4bac
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1341020326
##########
extensions/systemd/libwrapper/DlopenWrapper.cpp:
##########
@@ -72,7 +72,7 @@ class DlopenJournal : public Journal {
template<typename F>
F loadSymbol(const char* const symbol_name) {
// The cast below is supported by POSIX platforms. https://stackoverflow.com/a/1096349
- F const symbol = (F)dlsym(libhandle_.get(), symbol_name);
+ F const symbol = reinterpret_cast<F>(dlsym(libhandle_.get(), symbol_name));
Review Comment:
Good point, updated in 9d15d87a77f8e03e0863f8910fc540d9665b9d8f
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1341470484
##########
.clang-tidy:
##########
Review Comment:
You are right, I was thinking about it, but didn't want to remove it before discussing it. Even though there were a lot of false positives most of them were in a single file, and the check also found some valid cases, so I was not sure about removing it, but now I think it's better as this check does not add enough value to keep it.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-1891 Integrate google clang-tidy checks in CI [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1347419137
##########
extensions/librdkafka/ConsumeKafka.cpp:
##########
@@ -230,7 +230,8 @@ std::vector<std::unique_ptr<rd_kafka_message_t, utils::rd_kafka_message_deleter>
while (messages.size() < max_poll_records_ && elapsed < max_poll_time_milliseconds_) {
logger_->log_debug("Polling for new messages for %d milliseconds...", max_poll_time_milliseconds_.count());
std::unique_ptr<rd_kafka_message_t, utils::rd_kafka_message_deleter>
- message { rd_kafka_consumer_poll(consumer_.get(), std::chrono::duration_cast<std::chrono::milliseconds>(max_poll_time_milliseconds_ - elapsed).count()), utils::rd_kafka_message_deleter() };
+ message { rd_kafka_consumer_poll(consumer_.get(), gsl::narrow<int>(std::chrono::duration_cast<std::chrono::milliseconds>(max_poll_time_milliseconds_ - elapsed).count())),
+ utils::rd_kafka_message_deleter() };
Review Comment:
Updated in b47d7b26fee1043ded3732f9d20710a1fe9206e8
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1341006225
##########
extensions/expression-language/tests/ExpressionLanguageTests.cpp:
##########
@@ -1321,13 +1321,13 @@ TEST_CASE("Now Date", "[expressionNowDate]") {
TEST_CASE("Parse RFC3339 with Expression Language toDate") {
using date::sys_days;
using org::apache::nifi::minifi::utils::timeutils::parseRfc3339;
- using namespace date::literals;
+ using namespace date::literals; // NOLINT(google-build-using-namespace)
using namespace std::literals::chrono_literals;
using std::chrono::milliseconds;
- milliseconds expected_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s).time_since_epoch());
- milliseconds expected_tenth_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s + 100ms).time_since_epoch());
- milliseconds expected_milli_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s + 190ms).time_since_epoch());
+ milliseconds expected_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s).time_since_epoch()); // NOLINT(whitespace/braces)
+ milliseconds expected_tenth_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s + 100ms).time_since_epoch()); // NOLINT(whitespace/braces)
+ milliseconds expected_milli_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s + 190ms).time_since_epoch()); // NOLINT(whitespace/braces)
Review Comment:
I think the clang-tidy check returned false positive here as it recognized this call as a C-style cast and warned for it, this change was meant to avoid that warning.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1341017929
##########
extensions/sftp/client/SFTPClient.cpp:
##########
@@ -83,7 +83,7 @@ static const char* sftp_strerror(unsigned long err) { // NOLINT(runtime/int) un
}
}
-static SFTPError libssh2_sftp_error_to_sftp_error(unsigned long libssh2_sftp_error) { // NOLINT(runtime/int) unsigned long comes from libssh2 API
+static SFTPError libssh2_sftp_error_to_sftp_error(unsigned long libssh2_sftp_error) { // NOLINT(runtime/int,google-runtime-int) unsigned long comes from libssh2 API
Review Comment:
There were instances for both checks where only one of them gave a warning for this issue, so I kept both of them.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1341429719
##########
extensions/systemd/tests/ConsumeJournaldTest.cpp:
##########
@@ -90,7 +90,7 @@ struct TestJournal final : libwrapper::Journal {
}
int getCursor(gsl::owner<char*>* const cursor_out) noexcept override {
- *cursor_out = gsl::owner<char*>(strdup(std::to_string(consumed).c_str()));
+ *cursor_out = strdup(std::to_string(consumed).c_str()); // NOLINT(cppcoreguidelines-owning-memory)
Review Comment:
There was no example that assigned owner through a pointer parameter. The examples don't have an additional layer of indirection over the owner.
Could you double check that leaving the gsl::owner annotation in place doesn't solve the warning? The C-style cast one should probably be just disabled, due to the high number of false positives.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1341498042
##########
extensions/expression-language/tests/ExpressionLanguageTests.cpp:
##########
@@ -1321,13 +1321,13 @@ TEST_CASE("Now Date", "[expressionNowDate]") {
TEST_CASE("Parse RFC3339 with Expression Language toDate") {
using date::sys_days;
using org::apache::nifi::minifi::utils::timeutils::parseRfc3339;
- using namespace date::literals;
+ using namespace date::literals; // NOLINT(google-build-using-namespace)
using namespace std::literals::chrono_literals;
using std::chrono::milliseconds;
- milliseconds expected_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s).time_since_epoch());
- milliseconds expected_tenth_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s + 100ms).time_since_epoch());
- milliseconds expected_milli_second = std::chrono::floor<milliseconds>((sys_days(2023_y / 03 / 01) + 19h + 04min + 55s + 190ms).time_since_epoch());
+ milliseconds expected_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s).time_since_epoch()); // NOLINT(whitespace/braces)
+ milliseconds expected_tenth_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s + 100ms).time_since_epoch()); // NOLINT(whitespace/braces)
+ milliseconds expected_milli_second = std::chrono::floor<milliseconds>((sys_days{2023_y / 03 / 01} + 19h + 04min + 55s + 190ms).time_since_epoch()); // NOLINT(whitespace/braces)
Review Comment:
Reverted in b1d4a35c9a2021f2bd8d0fa750e2f2971f635cae
##########
extensions/standard-processors/processors/PutTCP.cpp:
##########
@@ -215,7 +215,7 @@ asio::awaitable<std::error_code> ConnectionHandler<SocketType>::establishNewConn
continue;
}
if (max_size_of_socket_send_buffer_)
- socket.lowest_layer().set_option(TcpSocket::send_buffer_size(*max_size_of_socket_send_buffer_));
+ socket.lowest_layer().set_option(TcpSocket::send_buffer_size(*max_size_of_socket_send_buffer_)); // NOLINT(google-readability-casting)
Review Comment:
Removed in b1d4a35c9a2021f2bd8d0fa750e2f2971f635cae
##########
extensions/systemd/tests/ConsumeJournaldTest.cpp:
##########
@@ -90,7 +90,7 @@ struct TestJournal final : libwrapper::Journal {
}
int getCursor(gsl::owner<char*>* const cursor_out) noexcept override {
- *cursor_out = gsl::owner<char*>(strdup(std::to_string(consumed).c_str()));
+ *cursor_out = strdup(std::to_string(consumed).c_str()); // NOLINT(cppcoreguidelines-owning-memory)
Review Comment:
Reverted in b1d4a35c9a2021f2bd8d0fa750e2f2971f635cae
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1341013782
##########
extensions/systemd/tests/ConsumeJournaldTest.cpp:
##########
@@ -90,7 +90,7 @@ struct TestJournal final : libwrapper::Journal {
}
int getCursor(gsl::owner<char*>* const cursor_out) noexcept override {
- *cursor_out = gsl::owner<char*>(strdup(std::to_string(consumed).c_str()));
+ *cursor_out = strdup(std::to_string(consumed).c_str()); // NOLINT(cppcoreguidelines-owning-memory)
Review Comment:
As the `cursor_out` parameter is already a `gsl::owner` type the cast and according to [these examples](https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/owning-memory.html) that should be enough to mark the returned pointer's onwership. The previous annotation was marked as a C-style cast, but I think the current `cppcoreguidelines-owning-memory` warning is also a false positive so it was suppressed.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1341008576
##########
extensions/standard-processors/processors/PutTCP.cpp:
##########
@@ -215,7 +215,7 @@ asio::awaitable<std::error_code> ConnectionHandler<SocketType>::establishNewConn
continue;
}
if (max_size_of_socket_send_buffer_)
- socket.lowest_layer().set_option(TcpSocket::send_buffer_size(*max_size_of_socket_send_buffer_));
+ socket.lowest_layer().set_option(TcpSocket::send_buffer_size(*max_size_of_socket_send_buffer_)); // NOLINT(google-readability-casting)
Review Comment:
Same false positive, clang-tidy recognizing `TcpSocket::send_buffer_size()` call as a C-style cast
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-1891 Integrate google clang-tidy checks in CI [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1367165495
##########
libminifi/src/core/ProcessSession.cpp:
##########
@@ -175,7 +175,7 @@ std::shared_ptr<core::FlowFile> ProcessSession::cloneDuringTransfer(const std::s
}
std::shared_ptr<core::FlowFile> ProcessSession::clone(const std::shared_ptr<core::FlowFile> &parent, int64_t offset, int64_t size) {
- if ((uint64_t) (offset + size) > parent->getSize()) {
+ if (static_cast<uint64_t>(offset + size) > parent->getSize()) {
Review Comment:
I would use `gsl::narrow` here, too
##########
extensions/sftp/tests/PutSFTPTests.cpp:
##########
@@ -201,7 +201,7 @@ class PutSFTPTestsFixture {
namespace {
std::size_t directoryContentCount(const std::filesystem::path& dir) {
- return (std::size_t)std::distance(std::filesystem::directory_iterator{dir}, std::filesystem::directory_iterator{});
+ return static_cast<std::size_t>(std::distance(std::filesystem::directory_iterator{dir}, std::filesystem::directory_iterator{}));
Review Comment:
`gsl::narrow` may be more appropriate here: if the distance is negative for some strange reason, it's better to crash than to return a huge number
##########
libminifi/src/core/flow/StructuredConfiguration.cpp:
##########
@@ -266,10 +266,10 @@ void StructuredConfiguration::parseProcessorNode(const Node& processors_node, co
logger_->log_debug("setting scheduling strategy as {}", procCfg.schedulingStrategy);
}
- int32_t maxConcurrentTasks;
+ int32_t maxConcurrentTasks = 0;
if (core::Property::StringToInt(procCfg.maxConcurrentTasks, maxConcurrentTasks)) {
logger_->log_debug("parseProcessorNode: maxConcurrentTasks => [{}]", maxConcurrentTasks);
- processor->setMaxConcurrentTasks((uint8_t) maxConcurrentTasks);
+ processor->setMaxConcurrentTasks(static_cast<uint8_t>(maxConcurrentTasks));
}
Review Comment:
would
```c++
uint8_t maxConcurrentTasks;
if (core::Property::StringToInt(procCfg.maxConcurrentTasks, maxConcurrentTasks)) {
logger_->log_debug("parseProcessorNode: maxConcurrentTasks => [{}]", maxConcurrentTasks);
processor->setMaxConcurrentTasks(maxConcurrentTasks);
}
```
work? I think that would be better, if it works.
##########
libminifi/src/provenance/Provenance.cpp:
##########
@@ -249,17 +249,17 @@ bool ProvenanceEventRecord::deserialize(io::InputStream &input_stream) {
}
}
- uint32_t eventType;
+ uint32_t eventType = 0;
{
const auto ret = input_stream.read(eventType);
if (ret != 4) {
return false;
}
}
- this->_eventType = (ProvenanceEventRecord::ProvenanceEventType) eventType;
+ _eventType = static_cast<ProvenanceEventRecord::ProvenanceEventType>(eventType);
Review Comment:
we could check this:
```suggestion
if (auto event_type_opt = magic_enum::enum_cast<ProvenanceEventRecord::ProvenanceEventType>(eventType)) {
_eventType = *event_type_opt;
} else {
return false;
}
```
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-1891 Integrate google clang-tidy checks in CI [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm closed pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-1891 Integrate google clang-tidy checks in CI [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1371279027
##########
libminifi/src/provenance/Provenance.cpp:
##########
@@ -249,17 +249,17 @@ bool ProvenanceEventRecord::deserialize(io::InputStream &input_stream) {
}
}
- uint32_t eventType;
+ uint32_t eventType = 0;
{
const auto ret = input_stream.read(eventType);
if (ret != 4) {
return false;
}
}
- this->_eventType = (ProvenanceEventRecord::ProvenanceEventType) eventType;
+ _eventType = static_cast<ProvenanceEventRecord::ProvenanceEventType>(eventType);
Review Comment:
Good point, updated in ff1c274db7dd0e6646c1fa4d25317f7552f5f157
##########
libminifi/src/core/flow/StructuredConfiguration.cpp:
##########
@@ -266,10 +266,10 @@ void StructuredConfiguration::parseProcessorNode(const Node& processors_node, co
logger_->log_debug("setting scheduling strategy as {}", procCfg.schedulingStrategy);
}
- int32_t maxConcurrentTasks;
+ int32_t maxConcurrentTasks = 0;
if (core::Property::StringToInt(procCfg.maxConcurrentTasks, maxConcurrentTasks)) {
logger_->log_debug("parseProcessorNode: maxConcurrentTasks => [{}]", maxConcurrentTasks);
- processor->setMaxConcurrentTasks((uint8_t) maxConcurrentTasks);
+ processor->setMaxConcurrentTasks(static_cast<uint8_t>(maxConcurrentTasks));
}
Review Comment:
Yes, good idea, updated in ff1c274db7dd0e6646c1fa4d25317f7552f5f157
##########
libminifi/src/core/ProcessSession.cpp:
##########
@@ -175,7 +175,7 @@ std::shared_ptr<core::FlowFile> ProcessSession::cloneDuringTransfer(const std::s
}
std::shared_ptr<core::FlowFile> ProcessSession::clone(const std::shared_ptr<core::FlowFile> &parent, int64_t offset, int64_t size) {
- if ((uint64_t) (offset + size) > parent->getSize()) {
+ if (static_cast<uint64_t>(offset + size) > parent->getSize()) {
Review Comment:
Updated in ff1c274db7dd0e6646c1fa4d25317f7552f5f157
##########
extensions/sftp/tests/PutSFTPTests.cpp:
##########
@@ -201,7 +201,7 @@ class PutSFTPTestsFixture {
namespace {
std::size_t directoryContentCount(const std::filesystem::path& dir) {
- return (std::size_t)std::distance(std::filesystem::directory_iterator{dir}, std::filesystem::directory_iterator{});
+ return static_cast<std::size_t>(std::distance(std::filesystem::directory_iterator{dir}, std::filesystem::directory_iterator{}));
Review Comment:
Updated in ff1c274db7dd0e6646c1fa4d25317f7552f5f157
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1341418984
##########
extensions/standard-processors/processors/PutTCP.cpp:
##########
@@ -215,7 +215,7 @@ asio::awaitable<std::error_code> ConnectionHandler<SocketType>::establishNewConn
continue;
}
if (max_size_of_socket_send_buffer_)
- socket.lowest_layer().set_option(TcpSocket::send_buffer_size(*max_size_of_socket_send_buffer_));
+ socket.lowest_layer().set_option(TcpSocket::send_buffer_size(*max_size_of_socket_send_buffer_)); // NOLINT(google-readability-casting)
Review Comment:
I think casts are pretty well covered by other checks. I would just disable this one due to the false positives, instead of littering the code with NOLINTs.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1671: MINIFICPP-1891 Integrate google clang-tidy checks in CI
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1671:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1671#discussion_r1341433299
##########
.clang-tidy:
##########
Review Comment:
Just a general comment: If a check produces many false positives, then we shouldn't enable it at all costs. The static analyzer is meant to help development, not hinder it. If a particular check hurts more than it helps, then we shouldn't use it.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org