You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/06/20 16:25:31 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request, #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
lordgamez opened a new pull request, #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354
https://issues.apache.org/jira/browse/MINIFICPP-1860
----------------------------------
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] lordgamez commented on a diff in pull request #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r923548689
##########
libminifi/test/unit/BackTraceTests.cpp:
##########
@@ -41,17 +41,13 @@ class WorkerNumberExecutions : public utils::AfterExecute<int> {
}
bool isFinished(const int &result) override {
- if (result > 0 && ++runs < tasks) {
- return false;
- } else {
- return true;
- }
+ return !(result > 0 && ++runs < tasks);
Review Comment:
I suppose it doesn't really make readability worse, so I updated it in 1e565fb3ed786137ba5410991273a85cf5102a6a
##########
libminifi/test/unit/FileUtilsTests.cpp:
##########
@@ -80,17 +80,17 @@ TEST_CASE("TestFileUtils::get_child_path", "[TestGetChildPath]") {
REQUIRE("bar\\" == FileUtils::get_child_path("C:\\foo\\bar\\"));
REQUIRE("foo" == FileUtils::get_child_path("C:\\foo"));
REQUIRE("foo\\" == FileUtils::get_child_path("C:\\foo\\"));
- REQUIRE("" == FileUtils::get_child_path("C:\\"));
- REQUIRE("" == FileUtils::get_child_path("C:\\\\"));
+ REQUIRE(FileUtils::get_child_path("C:\\").empty());
+ REQUIRE(FileUtils::get_child_path("C:\\\\").empty());
#else
REQUIRE("bar" == FileUtils::get_child_path("foo/bar"));
REQUIRE("bar/" == FileUtils::get_child_path("foo/bar/"));
REQUIRE("bar" == FileUtils::get_child_path("/foo/bar"));
REQUIRE("bar/" == FileUtils::get_child_path("/foo/bar/"));
REQUIRE("foo" == FileUtils::get_child_path("/foo"));
REQUIRE("foo/" == FileUtils::get_child_path("/foo/"));
- REQUIRE("" == FileUtils::get_child_path("/"));
- REQUIRE("" == FileUtils::get_child_path("//"));
+ REQUIRE(FileUtils::get_child_path("/").empty());
+ REQUIRE(FileUtils::get_child_path("//").empty());
Review Comment:
I agree, updated in 1e565fb3ed786137ba5410991273a85cf5102a6a
--
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] fgerlits commented on a diff in pull request #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
fgerlits commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r925685975
##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -254,7 +256,7 @@ void C2Agent::serializeMetrics(C2Payload &metric_payload, const std::string &nam
const auto payloads = std::count_if(begin(metrics), end(metrics), [](const state::response::SerializedResponseNode& metric) { return !metric.children.empty() || metric.keep_empty; });
metric_payload.reservePayloads(metric_payload.getNestedPayloads().size() + payloads);
for (const auto &metric : metrics) {
- if (metric.children.size() > 0 || (metric.children.size() == 0 && metric.keep_empty)) {
+ if (!metric.children.empty() || (metric.children.empty() && metric.keep_empty)) {
Review Comment:
minor, but I think this would read better (and would be slightly shorter) as
```suggestion
if (metric.keep_empty || !metric.children.empty()) {
```
##########
main/AgentDocs.h:
##########
@@ -26,7 +26,7 @@ class AgentDocs {
public:
void generate(const std::string &docsdir, std::ostream &genStream);
private:
- [[nodiscard]] inline std::string extractClassName(const std::string &processor) const;
+ [[nodiscard]] static inline std::string extractClassName(const std::string &processor);
Review Comment:
this isn't really `inline`
##########
extensions/standard-processors/tests/unit/GetFileTests.cpp:
##########
@@ -40,9 +40,9 @@ namespace {
class GetFileTestController {
public:
GetFileTestController();
- [[nodiscard]] std::string getFullPath(const std::string filename) const;
+ [[nodiscard]] std::string getFullPath(const std::string& filename) const;
[[nodiscard]] std::string getInputFilePath() const;
- void setProperty(const core::Property& property, const std::string& value);
+ void setProperty(const core::Property& property, const std::string& value) const;
Review Comment:
Does clang-tidy want us to mark `setProperty()` `const`? It modifies the state of the contained `test_plan_`, so I would not call it `const`. It can only be marked `const` because `test_plan_` is a pointer.
##########
libminifi/src/core/Processor.cpp:
##########
@@ -304,18 +303,13 @@ bool Processor::isThrottledByBackpressure() const {
}
return false;
})();
Review Comment:
`isThrottledByOutgoing` could be rewritten with `ranges::any_of` (or two of them) similar to `isForcedByIncomingCycle`
##########
extensions/http-curl/client/HTTPClient.cpp:
##########
@@ -437,29 +439,24 @@ void HTTPClient::setFollowRedirects(bool follow) {
}
bool HTTPClient::isValidHttpHeaderField(std::string_view field_name) {
- if (field_name.size() == 0) {
+ if (field_name.empty()) {
return false;
}
// RFC822 3.1.2: The field-name must be composed of printable ASCII characters
// (i.e., characters that have values between 33. and 126., decimal, except colon).
- for (auto ch : field_name) {
- if (ch < 33 || ch > 126 || ch == ':') {
- return false;
- }
- }
- return true;
+ return ranges::all_of(field_name, [](char c) { return c >= 33 && c <= 126 && c != ':'; });
}
std::string HTTPClient::replaceInvalidCharactersInHttpHeaderFieldName(std::string_view field_name) {
- if (field_name.size() == 0) {
+ if (field_name.empty()) {
return "X-MiNiFi-Empty-Attribute-Name";
}
std::string result;
// RFC822 3.1.2: The field-name must be composed of printable ASCII characters
// (i.e., characters that have values between 33. and 126., decimal, except colon).
- for (auto ch : field_name) {
+ for (auto ch : field_name) { // NOLINT(readability-use-anyofallof)
Review Comment:
Instead of the NOLINT, we could change this to a `transform` (also changing the parameter type to `std::string`):
```c++
#include "range/v3/action/transform.hpp"
// ...
ranges::actions::transform(field_name, [](char ch) {
return (ch >= 33 && ch <= 126 && ch != ':') ? ch : '-';
});
return field_name;
```
Or if you want to keep a `string_view`, then this works:
```c++
#include "range/v3/view/transform.hpp"
#include "range/v3/range/conversion.hpp"
// ...
return ranges::views::transform(field_name, [](char ch) {
return (ch >= 33 && ch <= 126 && ch != ':') ? ch : '-';
}) | ranges::to<std::string>();
```
although it's less nice IMO.
##########
libminifi/src/core/Processor.cpp:
##########
@@ -304,18 +303,13 @@ bool Processor::isThrottledByBackpressure() const {
}
return false;
})();
- bool isForcedByIncomingCycle = ([&] {
- for (auto &inConn : incoming_connections_) {
- auto connection = dynamic_cast<Connection*>(inConn);
- if (!connection) {
- continue;
- }
- if (partOfCycle(connection) && connection->isFull()) {
- return true;
- }
+ bool isForcedByIncomingCycle = ranges::any_of(incoming_connections_, [&](auto& inConn) {
+ auto connection = dynamic_cast<Connection*>(inConn);
+ if (!connection) {
+ return false;
}
- return false;
- })();
+ return partOfCycle(connection) && connection->isFull();
Review Comment:
this could be
```c++
return connection && partOfCycle(connection) && connection->isFull();
```
--
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 #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r926468331
##########
main/AgentDocs.h:
##########
@@ -26,7 +26,7 @@ class AgentDocs {
public:
void generate(const std::string &docsdir, std::ostream &genStream);
private:
- [[nodiscard]] inline std::string extractClassName(const std::string &processor) const;
+ [[nodiscard]] static inline std::string extractClassName(const std::string &processor);
Review Comment:
Updated in d3b1db61b23d80b28385fde5f77af73c4661b95a
##########
libminifi/src/core/Processor.cpp:
##########
@@ -304,18 +303,13 @@ bool Processor::isThrottledByBackpressure() const {
}
return false;
})();
Review Comment:
Good idea, updated in d3b1db61b23d80b28385fde5f77af73c4661b95a
--
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] adamdebreceni commented on a diff in pull request #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r923485879
##########
libminifi/test/unit/FileUtilsTests.cpp:
##########
@@ -80,17 +80,17 @@ TEST_CASE("TestFileUtils::get_child_path", "[TestGetChildPath]") {
REQUIRE("bar\\" == FileUtils::get_child_path("C:\\foo\\bar\\"));
REQUIRE("foo" == FileUtils::get_child_path("C:\\foo"));
REQUIRE("foo\\" == FileUtils::get_child_path("C:\\foo\\"));
- REQUIRE("" == FileUtils::get_child_path("C:\\"));
- REQUIRE("" == FileUtils::get_child_path("C:\\\\"));
+ REQUIRE(FileUtils::get_child_path("C:\\").empty());
+ REQUIRE(FileUtils::get_child_path("C:\\\\").empty());
#else
REQUIRE("bar" == FileUtils::get_child_path("foo/bar"));
REQUIRE("bar/" == FileUtils::get_child_path("foo/bar/"));
REQUIRE("bar" == FileUtils::get_child_path("/foo/bar"));
REQUIRE("bar/" == FileUtils::get_child_path("/foo/bar/"));
REQUIRE("foo" == FileUtils::get_child_path("/foo"));
REQUIRE("foo/" == FileUtils::get_child_path("/foo/"));
- REQUIRE("" == FileUtils::get_child_path("/"));
- REQUIRE("" == FileUtils::get_child_path("//"));
+ REQUIRE(FileUtils::get_child_path("/").empty());
+ REQUIRE(FileUtils::get_child_path("//").empty());
Review Comment:
I don't know about this change, `REQUIRE` destructures the expression and the error message is along the lines of "equality comparison failed, X != Y", with this change I think the error message will simply say that the expression is fails I think
--
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] adamdebreceni commented on a diff in pull request #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r923482642
##########
libminifi/test/unit/BackTraceTests.cpp:
##########
@@ -41,17 +41,13 @@ class WorkerNumberExecutions : public utils::AfterExecute<int> {
}
bool isFinished(const int &result) override {
- if (result > 0 && ++runs < tasks) {
- return false;
- } else {
- return true;
- }
+ return !(result > 0 && ++runs < tasks);
Review Comment:
should we distribute the `!` over `&&`?
```
result <= 0 || ++runs >= tasks
```
--
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 #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r903397575
##########
extensions/http-curl/client/HTTPClient.cpp:
##########
@@ -437,13 +438,13 @@ void HTTPClient::setFollowRedirects(bool follow) {
}
bool HTTPClient::isValidHttpHeaderField(std::string_view field_name) {
- if (field_name.size() == 0) {
+ if (field_name.empty()) {
return false;
}
// RFC822 3.1.2: The field-name must be composed of printable ASCII characters
// (i.e., characters that have values between 33. and 126., decimal, except colon).
- for (auto ch : field_name) {
+ for (auto ch : field_name) { // NOLINT(readability-use-anyofallof)
Review Comment:
Updated in 85e2a718a128ed49cdbac0e41d23628cf5a1a98c
--
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 #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r926570096
##########
extensions/standard-processors/tests/unit/GetFileTests.cpp:
##########
@@ -40,9 +40,9 @@ namespace {
class GetFileTestController {
public:
GetFileTestController();
- [[nodiscard]] std::string getFullPath(const std::string filename) const;
+ [[nodiscard]] std::string getFullPath(const std::string& filename) const;
[[nodiscard]] std::string getInputFilePath() const;
- void setProperty(const core::Property& property, const std::string& value);
+ void setProperty(const core::Property& property, const std::string& value) const;
Review Comment:
I agree, it should not mark in this case it according to the documentation: _this check will not suggest to add a const to a non-const method if the method reads a private member variable of pointer type because that allows to modify the pointee which might not preserve logical constness_ To avoid this I made the members private in 942ae453a3b45e8ac7eed232be0283e415a890de
--
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 #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r923548271
##########
libminifi/src/core/Connectable.cpp:
##########
@@ -76,11 +72,7 @@ bool Connectable::isSupportedRelationship(const core::Relationship &relationship
const auto conditionalLock = isConnectableRunning ? std::unique_lock<std::mutex>() : std::unique_lock<std::mutex>(relationship_mutex_);
const auto &it = relationships_.find(relationship.getName());
- if (it != relationships_.end()) {
- return true;
- } else {
- return false;
- }
+ return it != relationships_.end();
Review Comment:
Good idea, updated in 1e565fb3ed786137ba5410991273a85cf5102a6a
--
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 #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r927388710
##########
libminifi/src/core/Processor.cpp:
##########
@@ -290,32 +289,16 @@ bool Processor::partOfCycle(Connection* conn) {
}
bool Processor::isThrottledByBackpressure() const {
- bool isThrottledByOutgoing = ([&] {
- for (auto &outIt : outgoing_connections_) {
- for (auto &out : outIt.second) {
- auto connection = dynamic_cast<Connection*>(out);
- if (!connection) {
- continue;
- }
- if (connection->isFull()) {
- return true;
- }
- }
- }
- return false;
- })();
- bool isForcedByIncomingCycle = ([&] {
- for (auto &inConn : incoming_connections_) {
- auto connection = dynamic_cast<Connection*>(inConn);
- if (!connection) {
- continue;
- }
- if (partOfCycle(connection) && connection->isFull()) {
- return true;
- }
- }
- return false;
- })();
+ bool isThrottledByOutgoing = ranges::any_of(outgoing_connections_, [&](auto& outIt) {
+ return ranges::any_of(outIt.second, [&](auto* out) {
+ auto connection = dynamic_cast<Connection*>(out);
+ return connection && connection->isFull();
+ });
+ });
+ bool isForcedByIncomingCycle = ranges::any_of(incoming_connections_, [&](auto& inConn) {
+ auto connection = dynamic_cast<Connection*>(inConn);
+ return connection && partOfCycle(connection) && connection->isFull();
+ });
Review Comment:
Updated in c29515bfaea390b4b05c1796e962e6c02b9c6cf0
--
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] adamdebreceni commented on a diff in pull request #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r923472754
##########
libminifi/src/core/Connectable.cpp:
##########
@@ -76,11 +72,7 @@ bool Connectable::isSupportedRelationship(const core::Relationship &relationship
const auto conditionalLock = isConnectableRunning ? std::unique_lock<std::mutex>() : std::unique_lock<std::mutex>(relationship_mutex_);
const auto &it = relationships_.find(relationship.getName());
- if (it != relationships_.end()) {
- return true;
- } else {
- return false;
- }
+ return it != relationships_.end();
Review Comment:
should we further reduce this using `contains`? (and at line 101)
--
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] adamdebreceni commented on a diff in pull request #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r923485879
##########
libminifi/test/unit/FileUtilsTests.cpp:
##########
@@ -80,17 +80,17 @@ TEST_CASE("TestFileUtils::get_child_path", "[TestGetChildPath]") {
REQUIRE("bar\\" == FileUtils::get_child_path("C:\\foo\\bar\\"));
REQUIRE("foo" == FileUtils::get_child_path("C:\\foo"));
REQUIRE("foo\\" == FileUtils::get_child_path("C:\\foo\\"));
- REQUIRE("" == FileUtils::get_child_path("C:\\"));
- REQUIRE("" == FileUtils::get_child_path("C:\\\\"));
+ REQUIRE(FileUtils::get_child_path("C:\\").empty());
+ REQUIRE(FileUtils::get_child_path("C:\\\\").empty());
#else
REQUIRE("bar" == FileUtils::get_child_path("foo/bar"));
REQUIRE("bar/" == FileUtils::get_child_path("foo/bar/"));
REQUIRE("bar" == FileUtils::get_child_path("/foo/bar"));
REQUIRE("bar/" == FileUtils::get_child_path("/foo/bar/"));
REQUIRE("foo" == FileUtils::get_child_path("/foo"));
REQUIRE("foo/" == FileUtils::get_child_path("/foo/"));
- REQUIRE("" == FileUtils::get_child_path("/"));
- REQUIRE("" == FileUtils::get_child_path("//"));
+ REQUIRE(FileUtils::get_child_path("/").empty());
+ REQUIRE(FileUtils::get_child_path("//").empty());
Review Comment:
I don't know about this change, `REQUIRE` destructures the expression and the error message is along the lines of "equality comparison failed, X != Y", with this change I think the error message will simply say that the expression is false I think
--
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 #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r903391099
##########
extensions/http-curl/client/HTTPClient.cpp:
##########
@@ -437,13 +438,13 @@ void HTTPClient::setFollowRedirects(bool follow) {
}
bool HTTPClient::isValidHttpHeaderField(std::string_view field_name) {
- if (field_name.size() == 0) {
+ if (field_name.empty()) {
return false;
}
// RFC822 3.1.2: The field-name must be composed of printable ASCII characters
// (i.e., characters that have values between 33. and 126., decimal, except colon).
- for (auto ch : field_name) {
+ for (auto ch : field_name) { // NOLINT(readability-use-anyofallof)
Review Comment:
IMO this is a rare case where I would keep the comment to reference the RFC number and to keep the actual quote from the RFC.
--
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 #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r903204296
##########
extensions/http-curl/client/HTTPClient.cpp:
##########
@@ -437,13 +438,13 @@ void HTTPClient::setFollowRedirects(bool follow) {
}
bool HTTPClient::isValidHttpHeaderField(std::string_view field_name) {
- if (field_name.size() == 0) {
+ if (field_name.empty()) {
return false;
}
// RFC822 3.1.2: The field-name must be composed of printable ASCII characters
// (i.e., characters that have values between 33. and 126., decimal, except colon).
- for (auto ch : field_name) {
+ for (auto ch : field_name) { // NOLINT(readability-use-anyofallof)
Review Comment:
```suggestion
return std::all_of(std::begin(field_name), std::end(field_name), [](char c) { return c >= 33 && c <= 126 && c != ':'; });
```
--
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] fgerlits commented on a diff in pull request #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
fgerlits commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r926620090
##########
libminifi/src/core/Processor.cpp:
##########
@@ -290,32 +289,16 @@ bool Processor::partOfCycle(Connection* conn) {
}
bool Processor::isThrottledByBackpressure() const {
- bool isThrottledByOutgoing = ([&] {
- for (auto &outIt : outgoing_connections_) {
- for (auto &out : outIt.second) {
- auto connection = dynamic_cast<Connection*>(out);
- if (!connection) {
- continue;
- }
- if (connection->isFull()) {
- return true;
- }
- }
- }
- return false;
- })();
- bool isForcedByIncomingCycle = ([&] {
- for (auto &inConn : incoming_connections_) {
- auto connection = dynamic_cast<Connection*>(inConn);
- if (!connection) {
- continue;
- }
- if (partOfCycle(connection) && connection->isFull()) {
- return true;
- }
- }
- return false;
- })();
+ bool isThrottledByOutgoing = ranges::any_of(outgoing_connections_, [&](auto& outIt) {
+ return ranges::any_of(outIt.second, [&](auto* out) {
+ auto connection = dynamic_cast<Connection*>(out);
+ return connection && connection->isFull();
+ });
+ });
+ bool isForcedByIncomingCycle = ranges::any_of(incoming_connections_, [&](auto& inConn) {
+ auto connection = dynamic_cast<Connection*>(inConn);
+ return connection && partOfCycle(connection) && connection->isFull();
+ });
Review Comment:
This looks nice; just a few nitpicks:
* `outIt` is not an iterator; it should be `kv`, `key_value_pair`, `name_connection_set_pair` or similar
* I don't think any of these 3 lambdas need to capture anything
* the type of the parameters in lines 293 and 298 should be the same, and their names more similar.
--
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 #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r926469080
##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -254,7 +256,7 @@ void C2Agent::serializeMetrics(C2Payload &metric_payload, const std::string &nam
const auto payloads = std::count_if(begin(metrics), end(metrics), [](const state::response::SerializedResponseNode& metric) { return !metric.children.empty() || metric.keep_empty; });
metric_payload.reservePayloads(metric_payload.getNestedPayloads().size() + payloads);
for (const auto &metric : metrics) {
- if (metric.children.size() > 0 || (metric.children.size() == 0 && metric.keep_empty)) {
+ if (!metric.children.empty() || (metric.children.empty() && metric.keep_empty)) {
Review Comment:
Updated in d3b1db61b23d80b28385fde5f77af73c4661b95a
--
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] adamdebreceni commented on a diff in pull request #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r923558475
##########
libminifi/src/core/Connectable.cpp:
##########
@@ -76,11 +72,7 @@ bool Connectable::isSupportedRelationship(const core::Relationship &relationship
const auto conditionalLock = isConnectableRunning ? std::unique_lock<std::mutex>() : std::unique_lock<std::mutex>(relationship_mutex_);
const auto &it = relationships_.find(relationship.getName());
- if (it != relationships_.end()) {
- return true;
- } else {
- return false;
- }
+ return it != relationships_.end();
Review Comment:
line 101 could be eligible for the same transform
--
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 #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r926468702
##########
libminifi/src/core/Processor.cpp:
##########
@@ -304,18 +303,13 @@ bool Processor::isThrottledByBackpressure() const {
}
return false;
})();
- bool isForcedByIncomingCycle = ([&] {
- for (auto &inConn : incoming_connections_) {
- auto connection = dynamic_cast<Connection*>(inConn);
- if (!connection) {
- continue;
- }
- if (partOfCycle(connection) && connection->isFull()) {
- return true;
- }
+ bool isForcedByIncomingCycle = ranges::any_of(incoming_connections_, [&](auto& inConn) {
+ auto connection = dynamic_cast<Connection*>(inConn);
+ if (!connection) {
+ return false;
}
- return false;
- })();
+ return partOfCycle(connection) && connection->isFull();
Review Comment:
Good point, updated in d3b1db61b23d80b28385fde5f77af73c4661b95a
--
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 #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r926469345
##########
extensions/http-curl/client/HTTPClient.cpp:
##########
@@ -437,29 +439,24 @@ void HTTPClient::setFollowRedirects(bool follow) {
}
bool HTTPClient::isValidHttpHeaderField(std::string_view field_name) {
- if (field_name.size() == 0) {
+ if (field_name.empty()) {
return false;
}
// RFC822 3.1.2: The field-name must be composed of printable ASCII characters
// (i.e., characters that have values between 33. and 126., decimal, except colon).
- for (auto ch : field_name) {
- if (ch < 33 || ch > 126 || ch == ':') {
- return false;
- }
- }
- return true;
+ return ranges::all_of(field_name, [](char c) { return c >= 33 && c <= 126 && c != ':'; });
}
std::string HTTPClient::replaceInvalidCharactersInHttpHeaderFieldName(std::string_view field_name) {
- if (field_name.size() == 0) {
+ if (field_name.empty()) {
return "X-MiNiFi-Empty-Attribute-Name";
}
std::string result;
// RFC822 3.1.2: The field-name must be composed of printable ASCII characters
// (i.e., characters that have values between 33. and 126., decimal, except colon).
- for (auto ch : field_name) {
+ for (auto ch : field_name) { // NOLINT(readability-use-anyofallof)
Review Comment:
Good idea, updated in d3b1db61b23d80b28385fde5f77af73c4661b95a
--
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] fgerlits closed pull request #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
fgerlits closed pull request #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354
--
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] martinzink commented on a diff in pull request #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r903384308
##########
extensions/http-curl/client/HTTPClient.cpp:
##########
@@ -437,13 +438,13 @@ void HTTPClient::setFollowRedirects(bool follow) {
}
bool HTTPClient::isValidHttpHeaderField(std::string_view field_name) {
- if (field_name.size() == 0) {
+ if (field_name.empty()) {
return false;
}
// RFC822 3.1.2: The field-name must be composed of printable ASCII characters
// (i.e., characters that have values between 33. and 126., decimal, except colon).
- for (auto ch : field_name) {
+ for (auto ch : field_name) { // NOLINT(readability-use-anyofallof)
Review Comment:
If we are changing this I think we should name this lambda something like getPrintableASCIIChars and then we can remove the comment
--
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 #1354: MINIFICPP-1860 Integrate readability clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1354:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1354#discussion_r923561863
##########
libminifi/src/core/Connectable.cpp:
##########
@@ -76,11 +72,7 @@ bool Connectable::isSupportedRelationship(const core::Relationship &relationship
const auto conditionalLock = isConnectableRunning ? std::unique_lock<std::mutex>() : std::unique_lock<std::mutex>(relationship_mutex_);
const auto &it = relationships_.find(relationship.getName());
- if (it != relationships_.end()) {
- return true;
- } else {
- return false;
- }
+ return it != relationships_.end();
Review Comment:
Oop, I missed that, updated in latest commit.
--
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