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/09 12:43:11 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request, #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
lordgamez opened a new pull request, #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348
misc and modernize category checks of clang-tidy are fixed and added to CI. A few checks are only fixed in the changed files, as fixing them in the whole codebase would have blown up the PR significantly, but can be fixed over time when changing the problematic file in a PR. These checks are:
- modernize-concat-nested-namespaces
- modernize-use-auto
- modernize-use-nullptr
- modernize-pass-by-value
https://issues.apache.org/jira/browse/MINIFICPP-1833
----------------------------------------------------
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r897956678
##########
libminifi/src/utils/Id.cpp:
##########
@@ -193,8 +190,8 @@ IdGenerator::~IdGenerator() = default;
uint64_t IdGenerator::getDeviceSegmentFromString(const std::string& str, int numBits) const {
uint64_t deviceSegment = 0;
- for (size_t i = 0; i < str.length(); i++) {
- unsigned char c = toupper(str[i]);
+ for (auto ch : str) {
+ auto c = gsl::narrow<unsigned char>(std::toupper(gsl::narrow<unsigned char>(ch)));
Review Comment:
According to our previous discussion both should be okay, see [the discussion](https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r895969983) for more detail.
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r897965481
##########
libminifi/src/utils/Id.cpp:
##########
@@ -193,8 +190,8 @@ IdGenerator::~IdGenerator() = default;
uint64_t IdGenerator::getDeviceSegmentFromString(const std::string& str, int numBits) const {
uint64_t deviceSegment = 0;
- for (size_t i = 0; i < str.length(); i++) {
- unsigned char c = toupper(str[i]);
+ for (auto ch : str) {
+ auto c = gsl::narrow<unsigned char>(std::toupper(gsl::narrow<unsigned char>(ch)));
Review Comment:
No, my mistake, I didn't realize I used gsl::narrow instead of gsl::narrow_cast, bbab702554c6c6e0b462098cca1946d63b1d0eb0, thanks!
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r897373559
##########
extensions/libarchive/BinFiles.cpp:
##########
@@ -97,8 +97,8 @@ void BinFiles::preprocessFlowFile(core::ProcessContext* /*context*/, core::Proce
void BinManager::gatherReadyBins() {
std::lock_guard < std::mutex > lock(mutex_);
std::vector< std::string > emptyQueue;
- for (std::map<std::string, std::unique_ptr<std::deque<std::unique_ptr<Bin>>> >::iterator it=groupBinMap_.begin(); it !=groupBinMap_.end(); ++it) {
- std::unique_ptr < std::deque<std::unique_ptr<Bin>>>&queue = it->second;
+ for (auto& kvp : groupBinMap_) {
+ auto &queue = kvp.second;
Review Comment:
This is the perfect use case for structured bindings.
```suggestion
for (auto& [key, queue] : groupBinMap_) {
```
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r896806146
##########
extensions/coap/protocols/CoapC2Protocol.cpp:
##########
@@ -186,29 +181,30 @@ minifi::c2::Operation CoapProtocol::getOperation(int type) const {
return minifi::c2::Operation::PAUSE;
case 9:
return minifi::c2::Operation::RESUME;
+ default:
+ return minifi::c2::Operation::ACKNOWLEDGE;
Review Comment:
I'm not a fan of directly calling `abort`. I just found [`gsl_FailFast()`](https://github.com/gsl-lite/gsl-lite#contract-checking-configuration-macros), which is meant for exactly this purpose.
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r894217392
##########
libminifi/test/unit/EnvironmentUtilsTests.cpp:
##########
@@ -117,6 +117,9 @@ TEST_CASE("multithreaded environment manipulation", "[getenv][setenv][unsetenv]"
utils::Environment::unsetEnvironmentVariable(env_name.c_str());
break;
}
+ default: {
+ throw std::runtime_error("Operation value must be modulo 3");
Review Comment:
The idea was throwing an unhandled runtime_error which will terminate by default eventually. What would be the benefit of calling terminate explicitly?
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r895969983
##########
libminifi/src/utils/Id.cpp:
##########
@@ -193,8 +189,8 @@ IdGenerator::~IdGenerator() = default;
uint64_t IdGenerator::getDeviceSegmentFromString(const std::string& str, int numBits) const {
uint64_t deviceSegment = 0;
- for (size_t i = 0; i < str.length(); i++) {
- unsigned char c = toupper(str[i]);
+ for (auto ch : str) {
+ unsigned char c = toupper(ch);
Review Comment:
Good catch, thanks for bringing more attention to this!
As long as this is only used for UUID strings, it doesn't matter. This member is protected, so technically it could be called on non-UUID strings, but I don't think this is a huge concern. I suggest fixing this anyway for good measure.
If it is called with non-UUID strings, then a normal string can contain bytes that have their most significant bit set. I don't think we should terminate in that case. We can rely on unsigned integer overflow to convert those values from the -128-127 to the 0-256 range.
This is one of the rare cases when changing the value after a cast is not a programming error or undefined behavior (unlike signed integer overflow), but intentional, desired, and the simplest correct way. `gsl::narrow_cast` has the same behavior as `static_cast`, but can indicate that the narrowing is intentional. I'm fine with either `static_cast` or `narrow_cast`, since our tools don't flag explicit narrowing conversions.
This is what the Core Guidelines say about narrowing conversions in general: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es46-avoid-lossy-narrowing-truncating-arithmetic-conversions
And the cppreference page of toupper, with good examples: https://en.cppreference.com/w/cpp/string/byte/toupper
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r897969597
##########
libminifi/src/utils/Id.cpp:
##########
@@ -193,8 +190,8 @@ IdGenerator::~IdGenerator() = default;
uint64_t IdGenerator::getDeviceSegmentFromString(const std::string& str, int numBits) const {
uint64_t deviceSegment = 0;
- for (size_t i = 0; i < str.length(); i++) {
- unsigned char c = toupper(str[i]);
+ for (auto ch : str) {
+ auto c = gsl::narrow<unsigned char>(std::toupper(gsl::narrow<unsigned char>(ch)));
Review Comment:
@szaszm that was the reason for my mixup, but I actually wanted to use gsl::narrow_cast here, so I updated for that
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r895617857
##########
libminifi/test/unit/EnvironmentUtilsTests.cpp:
##########
@@ -117,6 +117,9 @@ TEST_CASE("multithreaded environment manipulation", "[getenv][setenv][unsetenv]"
utils::Environment::unsetEnvironmentVariable(env_name.c_str());
break;
}
+ default: {
+ throw std::runtime_error("Operation value must be modulo 3");
Review Comment:
If it terminates, then it's impossible to accidentally catch, log and proceed on a programming error. It would be best if it prevented compilation, but since this is a runtime condition, we can only detect the condition at runtime. Crashing the program can not be ignored, which should prevent releasing with a programming error.
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
fgerlits commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r895944533
##########
libminifi/src/utils/Id.cpp:
##########
@@ -193,8 +189,8 @@ IdGenerator::~IdGenerator() = default;
uint64_t IdGenerator::getDeviceSegmentFromString(const std::string& str, int numBits) const {
uint64_t deviceSegment = 0;
- for (size_t i = 0; i < str.length(); i++) {
- unsigned char c = toupper(str[i]);
+ for (auto ch : str) {
+ unsigned char c = toupper(ch);
Review Comment:
The argument of `toupper` needs to fit into an `unsigned char`. We have `static_cast<unsigned char>` elsewhere, but a `gsl::narrow` may 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
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r897965273
##########
libminifi/src/utils/Id.cpp:
##########
@@ -193,8 +190,8 @@ IdGenerator::~IdGenerator() = default;
uint64_t IdGenerator::getDeviceSegmentFromString(const std::string& str, int numBits) const {
uint64_t deviceSegment = 0;
- for (size_t i = 0; i < str.length(); i++) {
- unsigned char c = toupper(str[i]);
+ for (auto ch : str) {
+ auto c = gsl::narrow<unsigned char>(std::toupper(gsl::narrow<unsigned char>(ch)));
Review Comment:
@fgerlits suggested `gsl::narrow` originally. It shouldn't matter, since this function is working with a restricted set of characters. I suggested `gsl::narrow_cast` or `static_cast`, but since it hex digits only, @fgerlits commented that terminating is OK in that case. I don't think we should enforce that here, but I don't care that much.
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r895969983
##########
libminifi/src/utils/Id.cpp:
##########
@@ -193,8 +189,8 @@ IdGenerator::~IdGenerator() = default;
uint64_t IdGenerator::getDeviceSegmentFromString(const std::string& str, int numBits) const {
uint64_t deviceSegment = 0;
- for (size_t i = 0; i < str.length(); i++) {
- unsigned char c = toupper(str[i]);
+ for (auto ch : str) {
+ unsigned char c = toupper(ch);
Review Comment:
As long as this is only used for UUID strings, it doesn't matter. If it can be called with anything else, then a normal string can contain bytes that have their most significant bit set. I don't think we should terminate in that case. We can rely on unsigned integer overflow to convert those values from the -128-127 to the 0-256 range.
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r893699308
##########
libminifi/test/unit/EnvironmentUtilsTests.cpp:
##########
@@ -117,6 +117,9 @@ TEST_CASE("multithreaded environment manipulation", "[getenv][setenv][unsetenv]"
utils::Environment::unsetEnvironmentVariable(env_name.c_str());
break;
}
+ default: {
+ throw std::runtime_error("Operation value must be modulo 3");
Review Comment:
I think this should terminate. If it ever happens in the future, it's a programming error.
##########
libminifi/src/c2/ControllerSocketProtocol.cpp:
##########
@@ -237,6 +233,8 @@ void ControllerSocketProtocol::initialize(core::controller::ControllerServicePro
}
}
break;
+ default:
+ throw std::runtime_error("Unhandled operation: " + std::to_string(head));
Review Comment:
Who catches this?
##########
extensions/librdkafka/KafkaConnection.cpp:
##########
@@ -125,11 +121,9 @@ void KafkaConnection::logCallback(const rd_kafka_t* rk, int level, const char* /
case 7: // LOG_DEBUG
core::logging::LOG_DEBUG(logger) << buf;
break;
+ default:
+ throw std::runtime_error("Unknown log level: " + std::to_string(level));
Review Comment:
Again, I'd check if this makes sense. Terminate instead, if it's a programming error to have it happen?
##########
extensions/opc/src/opc.cpp:
##########
@@ -550,7 +546,7 @@ std::string OPCDateTime2String(UA_DateTime raw_date) {
int sz = snprintf(charBuf.data(), charBuf.size(), "%02hu-%02hu-%04hu %02hu:%02hu:%02hu.%03hu", dts.day, dts.month, dts.year, dts.hour, dts.min, dts.sec, dts.milliSec);
- return std::string(charBuf.data(), sz);
+ return {charBuf.data(), static_cast<std::size_t>(sz)};
Review Comment:
that `static_cast` should be `gsl::narrow` instead, possibly with checking for negative values.
##########
extensions/coap/protocols/CoapC2Protocol.cpp:
##########
@@ -186,29 +181,30 @@ minifi::c2::Operation CoapProtocol::getOperation(int type) const {
return minifi::c2::Operation::PAUSE;
case 9:
return minifi::c2::Operation::RESUME;
+ default:
+ return minifi::c2::Operation::ACKNOWLEDGE;
Review Comment:
Let's exclude this from the domain of the function and have the default case terminate. Or if you don't want to make the callers check, a wide contract could work as well (throwing on invalid values).
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r896784082
##########
libminifi/src/utils/Id.cpp:
##########
@@ -193,8 +189,8 @@ IdGenerator::~IdGenerator() = default;
uint64_t IdGenerator::getDeviceSegmentFromString(const std::string& str, int numBits) const {
uint64_t deviceSegment = 0;
- for (size_t i = 0; i < str.length(); i++) {
- unsigned char c = toupper(str[i]);
+ for (auto ch : str) {
+ unsigned char c = toupper(ch);
Review Comment:
Thanks for the info, updated in da2bf0f8cc4fd4748eb075a83196de5d6fe77c66
##########
extensions/opc/src/opc.cpp:
##########
@@ -550,7 +546,7 @@ std::string OPCDateTime2String(UA_DateTime raw_date) {
int sz = snprintf(charBuf.data(), charBuf.size(), "%02hu-%02hu-%04hu %02hu:%02hu:%02hu.%03hu", dts.day, dts.month, dts.year, dts.hour, dts.min, dts.sec, dts.milliSec);
- return std::string(charBuf.data(), sz);
+ return {charBuf.data(), static_cast<std::size_t>(sz)};
Review Comment:
Updated in da2bf0f8cc4fd4748eb075a83196de5d6fe77c66
##########
extensions/librdkafka/KafkaConnection.cpp:
##########
@@ -125,11 +121,9 @@ void KafkaConnection::logCallback(const rd_kafka_t* rk, int level, const char* /
case 7: // LOG_DEBUG
core::logging::LOG_DEBUG(logger) << buf;
break;
+ default:
+ throw std::runtime_error("Unknown log level: " + std::to_string(level));
Review Comment:
Updated in da2bf0f8cc4fd4748eb075a83196de5d6fe77c66
##########
extensions/coap/protocols/CoapC2Protocol.cpp:
##########
@@ -186,29 +181,30 @@ minifi::c2::Operation CoapProtocol::getOperation(int type) const {
return minifi::c2::Operation::PAUSE;
case 9:
return minifi::c2::Operation::RESUME;
+ default:
+ return minifi::c2::Operation::ACKNOWLEDGE;
Review Comment:
Updated in da2bf0f8cc4fd4748eb075a83196de5d6fe77c66
##########
libminifi/src/c2/ControllerSocketProtocol.cpp:
##########
@@ -237,6 +233,8 @@ void ControllerSocketProtocol::initialize(core::controller::ControllerServicePro
}
}
break;
+ default:
+ throw std::runtime_error("Unhandled operation: " + std::to_string(head));
Review Comment:
Replaced with a log in da2bf0f8cc4fd4748eb075a83196de5d6fe77c66
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r897636638
##########
extensions/libarchive/BinFiles.cpp:
##########
@@ -97,8 +97,8 @@ void BinFiles::preprocessFlowFile(core::ProcessContext* /*context*/, core::Proce
void BinManager::gatherReadyBins() {
std::lock_guard < std::mutex > lock(mutex_);
std::vector< std::string > emptyQueue;
- for (std::map<std::string, std::unique_ptr<std::deque<std::unique_ptr<Bin>>> >::iterator it=groupBinMap_.begin(); it !=groupBinMap_.end(); ++it) {
- std::unique_ptr < std::deque<std::unique_ptr<Bin>>>&queue = it->second;
+ for (auto& kvp : groupBinMap_) {
+ auto &queue = kvp.second;
Review Comment:
Good catch, updated in 51976a60612e6faebb5598bfebc611b3163a2be2
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r896875768
##########
extensions/coap/protocols/CoapC2Protocol.cpp:
##########
@@ -186,29 +181,30 @@ minifi::c2::Operation CoapProtocol::getOperation(int type) const {
return minifi::c2::Operation::PAUSE;
case 9:
return minifi::c2::Operation::RESUME;
+ default:
+ return minifi::c2::Operation::ACKNOWLEDGE;
Review Comment:
Good idea, thanks! Replaced `abort` in 40d67984d1fe1c036c5f78c32c2177a0a1deecb7
--
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 closed pull request #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
szaszm closed pull request #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r895969983
##########
libminifi/src/utils/Id.cpp:
##########
@@ -193,8 +189,8 @@ IdGenerator::~IdGenerator() = default;
uint64_t IdGenerator::getDeviceSegmentFromString(const std::string& str, int numBits) const {
uint64_t deviceSegment = 0;
- for (size_t i = 0; i < str.length(); i++) {
- unsigned char c = toupper(str[i]);
+ for (auto ch : str) {
+ unsigned char c = toupper(ch);
Review Comment:
Good catch, thanks for bringing more attention to this!
As long as this is only used for UUID strings, it doesn't matter. This member is protected, so technically it could be called on non-UUID strings, but I don't think this is a huge concern. If it is called with non-UUID strings, then a normal string can contain bytes that have their most significant bit set. I don't think we should terminate in that case. We can rely on unsigned integer overflow to convert those values from the -128-127 to the 0-256 range.
This is one of the rare cases when changing the value after a cast is not a programming error or undefined behavior (unlike signed integer overflow), but intentional, desired, and the simplest correct way. `gsl::narrow_cast` has the same behavior as `static_cast`, but can indicate that the narrowing is intentional. I'm fine with either `static_cast` or `narrow_cast`, since our tools don't flag explicit narrowing conversions.
This is what the Core Guidelines say about narrowing conversions in general: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es46-avoid-lossy-narrowing-truncating-arithmetic-conversions
And the cppreference page of toupper, with good examples: https://en.cppreference.com/w/cpp/string/byte/toupper
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r897951661
##########
libminifi/src/utils/Id.cpp:
##########
@@ -193,8 +190,8 @@ IdGenerator::~IdGenerator() = default;
uint64_t IdGenerator::getDeviceSegmentFromString(const std::string& str, int numBits) const {
uint64_t deviceSegment = 0;
- for (size_t i = 0; i < str.length(); i++) {
- unsigned char c = toupper(str[i]);
+ for (auto ch : str) {
+ auto c = gsl::narrow<unsigned char>(std::toupper(gsl::narrow<unsigned char>(ch)));
Review Comment:
is `gsl::narrow` the right choice here? `StringUtils` uses implicit conversions ([cppreference](https://en.cppreference.com/w/cpp/string/byte/toupper))
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r897959447
##########
libminifi/src/utils/Id.cpp:
##########
@@ -193,8 +190,8 @@ IdGenerator::~IdGenerator() = default;
uint64_t IdGenerator::getDeviceSegmentFromString(const std::string& str, int numBits) const {
uint64_t deviceSegment = 0;
- for (size_t i = 0; i < str.length(); i++) {
- unsigned char c = toupper(str[i]);
+ for (auto ch : str) {
+ auto c = gsl::narrow<unsigned char>(std::toupper(gsl::narrow<unsigned char>(ch)));
Review Comment:
the discussion mentions either `static_cast` or `gsl::narrow_cast`, `gsl::narrow` is different as it can throw, is that intentional?
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
fgerlits commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r896464586
##########
libminifi/src/utils/Id.cpp:
##########
@@ -193,8 +189,8 @@ IdGenerator::~IdGenerator() = default;
uint64_t IdGenerator::getDeviceSegmentFromString(const std::string& str, int numBits) const {
uint64_t deviceSegment = 0;
- for (size_t i = 0; i < str.length(); i++) {
- unsigned char c = toupper(str[i]);
+ for (auto ch : str) {
+ unsigned char c = toupper(ch);
Review Comment:
This is a hexadecimal digit coming from the `uid.minifi.device.segment` property in the config file, so terminating (not starting up) would be OK, but if we don't want to change the behavior in this refactoring PR, that's fine, too.
--
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 #1348: MINIFICPP-1833 - Integrate misc and modernize clang-tidy checks in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1348:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1348#discussion_r897636638
##########
extensions/libarchive/BinFiles.cpp:
##########
@@ -97,8 +97,8 @@ void BinFiles::preprocessFlowFile(core::ProcessContext* /*context*/, core::Proce
void BinManager::gatherReadyBins() {
std::lock_guard < std::mutex > lock(mutex_);
std::vector< std::string > emptyQueue;
- for (std::map<std::string, std::unique_ptr<std::deque<std::unique_ptr<Bin>>> >::iterator it=groupBinMap_.begin(); it !=groupBinMap_.end(); ++it) {
- std::unique_ptr < std::deque<std::unique_ptr<Bin>>>&queue = it->second;
+ for (auto& kvp : groupBinMap_) {
+ auto &queue = kvp.second;
Review Comment:
Good catch, updated in c067c00af8bb65f077173a4398705e59aec59339
--
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