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