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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
