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 2025/10/01 11:16:42 UTC
[PR] MINIFICPP-2643 Fix clang tidy issues [nifi-minifi-cpp]
lordgamez opened a new pull request, #2040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2040
https://issues.apache.org/jira/browse/MINIFICPP-2643
--------------
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
Re: [PR] MINIFICPP-2643 Fix clang tidy issues [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2040#discussion_r2406938808
##########
extensions/python/PythonBindings.cpp:
##########
@@ -84,7 +84,7 @@ PyInit_minifi_native(void) {
Py_INCREF(type.first);
}
const auto result = std::all_of(std::begin(types), std::end(types), [&](std::pair<PyTypeObject*, std::string_view> type) {
- return PyModule_AddObject(minifi_module_instance, type.second.data(), reinterpret_cast<PyObject*>(type.first)) == 0;
+ return PyModule_AddObject(minifi_module_instance, type.second.data(), reinterpret_cast<PyObject*>(type.first)) == 0; // NOLINT(bugprone-suspicious-stringview-data-usage)
Review Comment:
Replaced the std::string_view with std::string to make sure it is null-terminated in https://github.com/apache/nifi-minifi-cpp/pull/2040/commits/f66ce5c95061c24f82febd382375e4cec86c41fb
##########
libminifi/src/c2/HeartbeatJsonSerializer.cpp:
##########
@@ -28,7 +28,7 @@ namespace org::apache::nifi::minifi::c2 {
static void serializeOperationInfo(rapidjson::Value& target, const C2Payload& payload, rapidjson::Document::AllocatorType& alloc) {
gsl_Expects(target.IsObject());
- target.AddMember("operation", rapidjson::Value(magic_enum::enum_name<Operation>(payload.getOperation()).data(), alloc), alloc);
+ target.AddMember("operation", rapidjson::Value(magic_enum::enum_name<Operation>(payload.getOperation()).data(), alloc), alloc); // NOLINT(bugprone-suspicious-stringview-data-usage)
Review Comment:
Added comment that magic_enum ensures null-terminated strings in this case in https://github.com/apache/nifi-minifi-cpp/pull/2040/commits/f66ce5c95061c24f82febd382375e4cec86c41fb Unfortunately I could not ensure it with a valid static_assert
##########
libminifi/test/unit/SiteToSiteTests.cpp:
##########
@@ -236,9 +236,8 @@ TEST_CASE("TestSiteToSiteVerifySend using flowfile data", "[S2S]") {
TEST_CASE("TestSiteToSiteVerifyNegotiationFail", "[S2S]") {
auto collector = std::make_unique<SiteToSiteResponder>();
- const char negotiated_abort_code = magic_enum::enum_underlying(sitetosite::ResourceNegotiationStatusCode::NEGOTIATED_ABORT);
- std::string resp_code;
- resp_code.insert(resp_code.begin(), negotiated_abort_code);
+ const auto negotiated_abort_code = magic_enum::enum_underlying(sitetosite::ResourceNegotiationStatusCode::NEGOTIATED_ABORT);
+ const std::string resp_code{reinterpret_cast<const char*>(&negotiated_abort_code), 1};
Review Comment:
Updated in https://github.com/apache/nifi-minifi-cpp/pull/2040/commits/f66ce5c95061c24f82febd382375e4cec86c41fb
--
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-2643 Fix clang tidy issues [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #2040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2040#issuecomment-3414847271
After posting my +1, I've fixed the issue with raw new in ClassLoader + ObjectFactory. Please review my commit before merging.
--
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-2643 Fix clang tidy issues [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2040#discussion_r2401893044
##########
libminifi/include/core/state/UpdatePolicy.h:
##########
@@ -94,7 +89,7 @@ class UpdatePolicy {
class UpdatePolicyBuilder {
public:
static std::unique_ptr<UpdatePolicyBuilder> newBuilder(bool enable_all = false) {
- std::unique_ptr<UpdatePolicyBuilder> policy = std::unique_ptr<UpdatePolicyBuilder>( new UpdatePolicyBuilder(enable_all));
+ std::unique_ptr<UpdatePolicyBuilder> policy = std::unique_ptr<UpdatePolicyBuilder>( new UpdatePolicyBuilder(enable_all)); // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
Review Comment:
The UpdatePolicyBuilder's constructor is protected and make_unique cannot access 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-2643 Fix clang tidy issues [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2040#discussion_r2395320296
##########
libminifi/include/core/state/UpdatePolicy.h:
##########
@@ -94,7 +89,7 @@ class UpdatePolicy {
class UpdatePolicyBuilder {
public:
static std::unique_ptr<UpdatePolicyBuilder> newBuilder(bool enable_all = false) {
- std::unique_ptr<UpdatePolicyBuilder> policy = std::unique_ptr<UpdatePolicyBuilder>( new UpdatePolicyBuilder(enable_all));
+ std::unique_ptr<UpdatePolicyBuilder> policy = std::unique_ptr<UpdatePolicyBuilder>( new UpdatePolicyBuilder(enable_all)); // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
Review Comment:
why not change to make_unique?
##########
extensions/python/PythonBindings.cpp:
##########
@@ -84,7 +84,7 @@ PyInit_minifi_native(void) {
Py_INCREF(type.first);
}
const auto result = std::all_of(std::begin(types), std::end(types), [&](std::pair<PyTypeObject*, std::string_view> type) {
- return PyModule_AddObject(minifi_module_instance, type.second.data(), reinterpret_cast<PyObject*>(type.first)) == 0;
+ return PyModule_AddObject(minifi_module_instance, type.second.data(), reinterpret_cast<PyObject*>(type.first)) == 0; // NOLINT(bugprone-suspicious-stringview-data-usage)
Review Comment:
This warning seems valid: the usage seems to assume a null-terminated string, which a string_view is not guaranteed to be. We should add a comment explaining how it's guaranteed that the string_view is null-terminated, or fix the bug if it's not.
##########
libminifi/test/unit/SiteToSiteTests.cpp:
##########
@@ -236,9 +236,8 @@ TEST_CASE("TestSiteToSiteVerifySend using flowfile data", "[S2S]") {
TEST_CASE("TestSiteToSiteVerifyNegotiationFail", "[S2S]") {
auto collector = std::make_unique<SiteToSiteResponder>();
- const char negotiated_abort_code = magic_enum::enum_underlying(sitetosite::ResourceNegotiationStatusCode::NEGOTIATED_ABORT);
- std::string resp_code;
- resp_code.insert(resp_code.begin(), negotiated_abort_code);
+ const auto negotiated_abort_code = magic_enum::enum_underlying(sitetosite::ResourceNegotiationStatusCode::NEGOTIATED_ABORT);
+ const std::string resp_code{reinterpret_cast<const char*>(&negotiated_abort_code), 1};
Review Comment:
I'd prefer to do this with no reinterpret_cast, maybe like this:
```suggestion
const std::string resp_code{static_cast<char>(negotiated_abort_code)};
```
##########
libminifi/src/c2/HeartbeatJsonSerializer.cpp:
##########
@@ -28,7 +28,7 @@ namespace org::apache::nifi::minifi::c2 {
static void serializeOperationInfo(rapidjson::Value& target, const C2Payload& payload, rapidjson::Document::AllocatorType& alloc) {
gsl_Expects(target.IsObject());
- target.AddMember("operation", rapidjson::Value(magic_enum::enum_name<Operation>(payload.getOperation()).data(), alloc), alloc);
+ target.AddMember("operation", rapidjson::Value(magic_enum::enum_name<Operation>(payload.getOperation()).data(), alloc), alloc); // NOLINT(bugprone-suspicious-stringview-data-usage)
Review Comment:
I'd add a comment explaining that the enum names are static null-terminated strings behind the string_view, and add a static_assert checking that for a few (or all) enum values, just as code documentation.
--
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-2643 Fix clang tidy issues [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2040#discussion_r2435744095
##########
libminifi/include/core/state/UpdatePolicy.h:
##########
@@ -94,7 +89,7 @@ class UpdatePolicy {
class UpdatePolicyBuilder {
public:
static std::unique_ptr<UpdatePolicyBuilder> newBuilder(bool enable_all = false) {
- std::unique_ptr<UpdatePolicyBuilder> policy = std::unique_ptr<UpdatePolicyBuilder>( new UpdatePolicyBuilder(enable_all));
+ std::unique_ptr<UpdatePolicyBuilder> policy = std::unique_ptr<UpdatePolicyBuilder>( new UpdatePolicyBuilder(enable_all)); // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
Review Comment:
I think you are mixing up the `UpdatePolicyBuilder` and the `UpdatePolicy` classes. We are instantiating an `UpdatePolicyBuilder` here and its bool ctor is also protected on line 113.
--
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-2643 Fix clang tidy issues [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2040#discussion_r2394554941
##########
libminifi/test/libtest/integration/ConnectionCountingServer.cpp:
##########
@@ -85,14 +85,13 @@ void ReverseBodyPostHandler::saveConnectionId(struct mg_connection* conn) {
AddIdToUserConnectionData::AddIdToUserConnectionData() {
init_connection = [](const struct mg_connection*, void** user_connection_data) -> int {
- minifi::utils::SmallString<36>* id = new minifi::utils::SmallString<36>(minifi::utils::IdGenerator::getIdGenerator()->generate().to_string());
+ auto id = new minifi::utils::SmallString<36>(minifi::utils::IdGenerator::getIdGenerator()->generate().to_string());
*user_connection_data = reinterpret_cast<void*>(id);
return 0;
};
connection_close = [](const struct mg_connection* conn) -> void {
- auto user_connection_data = reinterpret_cast<minifi::utils::SmallString<36>*>(mg_get_user_connection_data(conn));
- delete user_connection_data;
+ std::ignore = std::unique_ptr<minifi::utils::SmallString<36>>(reinterpret_cast<minifi::utils::SmallString<36>*>(mg_get_user_connection_data(conn)));
Review Comment:
Updated in https://github.com/apache/nifi-minifi-cpp/pull/2040/commits/69c49f015f6818bc26b2af897f5ccf8a9bed0475
--
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-2643 Fix clang tidy issues [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2040#discussion_r2436655102
##########
libminifi/include/core/state/UpdatePolicy.h:
##########
@@ -94,7 +89,7 @@ class UpdatePolicy {
class UpdatePolicyBuilder {
public:
static std::unique_ptr<UpdatePolicyBuilder> newBuilder(bool enable_all = false) {
- std::unique_ptr<UpdatePolicyBuilder> policy = std::unique_ptr<UpdatePolicyBuilder>( new UpdatePolicyBuilder(enable_all));
+ std::unique_ptr<UpdatePolicyBuilder> policy = std::unique_ptr<UpdatePolicyBuilder>( new UpdatePolicyBuilder(enable_all)); // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
Review Comment:
You're right, I mixed them up. Would it be feasible to enable public move of UpdatePolicyBuilder, and make the change I suggested above?
--
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-2643 Fix clang tidy issues [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2040#discussion_r2436655102
##########
libminifi/include/core/state/UpdatePolicy.h:
##########
@@ -94,7 +89,7 @@ class UpdatePolicy {
class UpdatePolicyBuilder {
public:
static std::unique_ptr<UpdatePolicyBuilder> newBuilder(bool enable_all = false) {
- std::unique_ptr<UpdatePolicyBuilder> policy = std::unique_ptr<UpdatePolicyBuilder>( new UpdatePolicyBuilder(enable_all));
+ std::unique_ptr<UpdatePolicyBuilder> policy = std::unique_ptr<UpdatePolicyBuilder>( new UpdatePolicyBuilder(enable_all)); // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
Review Comment:
You're right, I mixed them up. Would it be feasible to enable public move of UpdatePolicyBuilder, and make the change I suggested above?
--
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-2643 Fix clang tidy issues [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm closed pull request #2040: MINIFICPP-2643 Fix clang tidy issues
URL: https://github.com/apache/nifi-minifi-cpp/pull/2040
--
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-2643 Fix clang tidy issues [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #2040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2040#discussion_r2394340315
##########
libminifi/test/libtest/integration/ConnectionCountingServer.cpp:
##########
@@ -85,14 +85,13 @@ void ReverseBodyPostHandler::saveConnectionId(struct mg_connection* conn) {
AddIdToUserConnectionData::AddIdToUserConnectionData() {
init_connection = [](const struct mg_connection*, void** user_connection_data) -> int {
- minifi::utils::SmallString<36>* id = new minifi::utils::SmallString<36>(minifi::utils::IdGenerator::getIdGenerator()->generate().to_string());
+ auto id = new minifi::utils::SmallString<36>(minifi::utils::IdGenerator::getIdGenerator()->generate().to_string());
*user_connection_data = reinterpret_cast<void*>(id);
return 0;
};
connection_close = [](const struct mg_connection* conn) -> void {
- auto user_connection_data = reinterpret_cast<minifi::utils::SmallString<36>*>(mg_get_user_connection_data(conn));
- delete user_connection_data;
+ std::ignore = std::unique_ptr<minifi::utils::SmallString<36>>(reinterpret_cast<minifi::utils::SmallString<36>*>(mg_get_user_connection_data(conn)));
Review Comment:
it's a bit weird to create a `unique_ptr` only to destruct it immediately; I think keeping the `delete` with a NOLINT comment would be better
--
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-2643 Fix clang tidy issues [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2040#discussion_r2432859109
##########
libminifi/include/core/state/UpdatePolicy.h:
##########
@@ -94,7 +89,7 @@ class UpdatePolicy {
class UpdatePolicyBuilder {
public:
static std::unique_ptr<UpdatePolicyBuilder> newBuilder(bool enable_all = false) {
- std::unique_ptr<UpdatePolicyBuilder> policy = std::unique_ptr<UpdatePolicyBuilder>( new UpdatePolicyBuilder(enable_all));
+ std::unique_ptr<UpdatePolicyBuilder> policy = std::unique_ptr<UpdatePolicyBuilder>( new UpdatePolicyBuilder(enable_all)); // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
Review Comment:
I see the bool ctor is public, and I see no reason not to make the move constructor public too. Why not do that and avoid an explicit new?
```suggestion
auto policy = std::unique_ptr<UpdatePolicyBuilder>(UpdatePolicyBuilder(enable_all));
```
##########
libminifi/include/core/state/UpdatePolicy.h:
##########
@@ -107,7 +102,7 @@ class UpdatePolicyBuilder {
}
std::unique_ptr<UpdatePolicy> build() {
- std::unique_ptr<UpdatePolicy> new_policy = std::unique_ptr<UpdatePolicy>(new UpdatePolicy(*(current_policy_.get())));
+ std::unique_ptr<UpdatePolicy> new_policy = std::unique_ptr<UpdatePolicy>(new UpdatePolicy(*(current_policy_.get()))); // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
Review Comment:
```suggestion
auto new_policy = std::unique_ptr<UpdatePolicy>(UpdatePolicy(*current_policy_));
```
--
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-2643 Fix clang tidy issues [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2040:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2040#discussion_r2435751465
##########
libminifi/include/core/state/UpdatePolicy.h:
##########
@@ -107,7 +102,7 @@ class UpdatePolicyBuilder {
}
std::unique_ptr<UpdatePolicy> build() {
- std::unique_ptr<UpdatePolicy> new_policy = std::unique_ptr<UpdatePolicy>(new UpdatePolicy(*(current_policy_.get())));
+ std::unique_ptr<UpdatePolicy> new_policy = std::unique_ptr<UpdatePolicy>(new UpdatePolicy(*(current_policy_.get()))); // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
Review Comment:
Here the comment says "Defines an immutable policy map" where the copy constructor is protected on purpose, with the UpdatePolicyBuilder defined as a friend class. I'm not sure this is a good design or if it is still valid, but I think refactoring the `UpdatePolicy` and `UpdatePolicyBuilder` classes should be figured out separately and it is out of scope of this PR.
--
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