[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-06 Thread GitBox 


adamdebreceni commented on a change in pull request #964:
URL: https://github.com/apache/nifi-minifi-cpp/pull/964#discussion_r552445577



##
File path: libminifi/src/utils/StringUtils.cpp
##
@@ -94,37 +94,39 @@ bool StringUtils::StringToFloat(std::string input, float 
, FailurePolicy
   return true;
 }
 
-std::string StringUtils::replaceEnvironmentVariables(std::string& 
original_string) {
-  int32_t beg_seq = 0;
-  int32_t end_seq = 0;
-  std::string source_string = original_string;
+std::string StringUtils::replaceEnvironmentVariables(std::string 
source_string) {
+  std::string::size_type beg_seq = 0;
+  std::string::size_type end_seq = 0;
   do {
 beg_seq = source_string.find("${", beg_seq);
+if (beg_seq == std::string::npos) {
+  break;
+}
 if (beg_seq > 0 && source_string.at(beg_seq - 1) == '\\') {
   beg_seq += 2;
   continue;
 }
-if (beg_seq < 0)
-  break;
 end_seq = source_string.find("}", beg_seq + 2);
-if (end_seq < 0)
+if (end_seq == std::string::npos) {
   break;
-if (end_seq - (beg_seq + 2) < 0) {
+}
+if (end_seq <= beg_seq + 2) {
   beg_seq += 2;
   continue;
 }
-const std::string env_field = source_string.substr(beg_seq + 2, end_seq - 
(beg_seq + 2));
-const std::string env_field_wrapped = source_string.substr(beg_seq, 
end_seq + 1);
-if (env_field.empty()) {
+auto env_var_length = end_seq - (beg_seq + 2);
+const std::string env_var = source_string.substr(beg_seq + 2, 
env_var_length);
+const std::string env_var_wrapped = source_string.substr(beg_seq, 
env_var_length + 3);
+if (env_var.empty()) {

Review comment:
   it seems like this condition is not possible anymore given 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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


adamdebreceni commented on a change in pull request #964:
URL: https://github.com/apache/nifi-minifi-cpp/pull/964#discussion_r551985634



##
File path: nanofi/include/sitetosite/CPeer.h
##
@@ -88,7 +88,7 @@ static void setPort(struct SiteToSiteCPeer * peer, uint16_t 
port) {
   peer->_port = port;
   if(peer->_url != NULL) {
 int i;
-for(i = strlen(peer->_url) -1; i >= 0; --i) {  // look for the last ':' in 
the string
+for(i = (int)(strlen(peer->_url)) - 1; i >= 0; --i) {  // look for the 
last ':' in the string

Review comment:
   the google coding guideline suggests using `static_cast` here (some 
other places touched by this PR also use C-style casts in the old code)





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


adamdebreceni commented on a change in pull request #964:
URL: https://github.com/apache/nifi-minifi-cpp/pull/964#discussion_r551902820



##
File path: main/AgentDocs.cpp
##
@@ -47,7 +47,7 @@ std::string AgentDocs::extractClassName(const std::string 
) const {
   auto lastOfIdx = processor.find_last_of(".");
   if (lastOfIdx != std::string::npos) {
 lastOfIdx++;  // if a value is found, increment to move beyond the .
-int nameLength = processor.length() - lastOfIdx;
+size_t nameLength = processor.length() - lastOfIdx;
 std::string processorName = processor.substr(lastOfIdx, nameLength);

Review comment:
   similarly to the ones you replaced, the `nameLength` is likewise 
unnecessary





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


adamdebreceni commented on a change in pull request #964:
URL: https://github.com/apache/nifi-minifi-cpp/pull/964#discussion_r551912512



##
File path: nanofi/include/sitetosite/CPeer.h
##
@@ -79,15 +79,15 @@ static void setHostName(struct SiteToSiteCPeer * peer, 
const char * hostname) {
   peer->_host[host_len] = '\0';
   peer->_url[host_len + 7] = ':';
   if(peer->_port != 0) {
-snprintf(peer->_url + host_len + 8, 5, "%d", peer->_port);
+snprintf(peer->_url + host_len + 8, 6, "%d", peer->_port);
   }
   return;
 }
 
 static void setPort(struct SiteToSiteCPeer * peer, uint16_t port) {
   peer->_port = port;
   if(peer->_url != NULL) {
-int i;
+size_t i;
 for(i = strlen(peer->_url) -1; i >= 0; --i) {  // look for the last ':' in 
the string

Review comment:
   won't this result in an infinite loop (or rather out of bound indexing), 
if the `_url` does not contain a `:`?





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


adamdebreceni commented on a change in pull request #964:
URL: https://github.com/apache/nifi-minifi-cpp/pull/964#discussion_r551912512



##
File path: nanofi/include/sitetosite/CPeer.h
##
@@ -79,15 +79,15 @@ static void setHostName(struct SiteToSiteCPeer * peer, 
const char * hostname) {
   peer->_host[host_len] = '\0';
   peer->_url[host_len + 7] = ':';
   if(peer->_port != 0) {
-snprintf(peer->_url + host_len + 8, 5, "%d", peer->_port);
+snprintf(peer->_url + host_len + 8, 6, "%d", peer->_port);
   }
   return;
 }
 
 static void setPort(struct SiteToSiteCPeer * peer, uint16_t port) {
   peer->_port = port;
   if(peer->_url != NULL) {
-int i;
+size_t i;
 for(i = strlen(peer->_url) -1; i >= 0; --i) {  // look for the last ':' in 
the string

Review comment:
   won't this result in an infinite loop (and out of bound indexing), if 
the `_url` does not contain a `:`?





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


adamdebreceni commented on a change in pull request #964:
URL: https://github.com/apache/nifi-minifi-cpp/pull/964#discussion_r551912512



##
File path: nanofi/include/sitetosite/CPeer.h
##
@@ -79,15 +79,15 @@ static void setHostName(struct SiteToSiteCPeer * peer, 
const char * hostname) {
   peer->_host[host_len] = '\0';
   peer->_url[host_len + 7] = ':';
   if(peer->_port != 0) {
-snprintf(peer->_url + host_len + 8, 5, "%d", peer->_port);
+snprintf(peer->_url + host_len + 8, 6, "%d", peer->_port);
   }
   return;
 }
 
 static void setPort(struct SiteToSiteCPeer * peer, uint16_t port) {
   peer->_port = port;
   if(peer->_url != NULL) {
-int i;
+size_t i;
 for(i = strlen(peer->_url) -1; i >= 0; --i) {  // look for the last ':' in 
the string

Review comment:
   won't this result in an infinite loop?





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


adamdebreceni commented on a change in pull request #964:
URL: https://github.com/apache/nifi-minifi-cpp/pull/964#discussion_r551912172



##
File path: nanofi/tests/CTestsBase.h
##
@@ -138,7 +138,8 @@ class TestControllerWithTemporaryWorkingDirectory {
   }
 
   ~TestControllerWithTemporaryWorkingDirectory() {
-chdir(old_cwd_);
+int success = chdir(old_cwd_);
+(void) success;

Review comment:
   is this added to silence an "unused return value" warning?





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


adamdebreceni commented on a change in pull request #964:
URL: https://github.com/apache/nifi-minifi-cpp/pull/964#discussion_r551902820



##
File path: main/AgentDocs.cpp
##
@@ -47,7 +47,7 @@ std::string AgentDocs::extractClassName(const std::string 
) const {
   auto lastOfIdx = processor.find_last_of(".");
   if (lastOfIdx != std::string::npos) {
 lastOfIdx++;  // if a value is found, increment to move beyond the .
-int nameLength = processor.length() - lastOfIdx;
+size_t nameLength = processor.length() - lastOfIdx;
 std::string processorName = processor.substr(lastOfIdx, nameLength);

Review comment:
   similarly to the ones you replaced the `nameLength` is likewise 
unnecessary





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


adamdebreceni commented on a change in pull request #964:
URL: https://github.com/apache/nifi-minifi-cpp/pull/964#discussion_r551902469



##
File path: libminifi/test/unit/ZlibStreamTests.cpp
##
@@ -97,7 +106,8 @@ TEST_CASE("gzip compression and decompression pipeline", 
"[basic]") {
 for (size_t i = 0U; i < 1024U; i++) {
   std::generate(buf.begin(), buf.end(), [&](){return dist(gen);});
   original += std::string(reinterpret_cast(buf.data()), 
buf.size());
-  REQUIRE(buf.size() == compressStream.write(buf.data(), buf.size()));
+  int buffer_size = gsl::narrow(buf.size());
+  REQUIRE(buffer_size == compressStream.write(buf.data(), 
gsl::narrow(buffer_size)));

Review comment:
   the `gsl::narrow` at line `110` is not necessary 





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


adamdebreceni commented on a change in pull request #964:
URL: https://github.com/apache/nifi-minifi-cpp/pull/964#discussion_r551895592



##
File path: libminifi/src/utils/StringUtils.cpp
##
@@ -95,21 +95,23 @@ bool StringUtils::StringToFloat(std::string input, float 
, FailurePolicy
 }
 
 std::string StringUtils::replaceEnvironmentVariables(std::string& 
original_string) {
-  int32_t beg_seq = 0;
-  int32_t end_seq = 0;
+  size_t beg_seq = 0;

Review comment:
   should we use `std::string::size_type`?





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


adamdebreceni commented on a change in pull request #964:
URL: https://github.com/apache/nifi-minifi-cpp/pull/964#discussion_r551893946



##
File path: libminifi/src/utils/StringUtils.cpp
##
@@ -95,21 +95,23 @@ bool StringUtils::StringToFloat(std::string input, float 
, FailurePolicy
 }
 
 std::string StringUtils::replaceEnvironmentVariables(std::string& 
original_string) {
-  int32_t beg_seq = 0;
-  int32_t end_seq = 0;
+  size_t beg_seq = 0;
+  size_t end_seq = 0;
   std::string source_string = original_string;
   do {
 beg_seq = source_string.find("${", beg_seq);
+if (beg_seq == std::string::npos) {
+  break;
+}
 if (beg_seq > 0 && source_string.at(beg_seq - 1) == '\\') {
   beg_seq += 2;
   continue;
 }
-if (beg_seq < 0)
-  break;
 end_seq = source_string.find("}", beg_seq + 2);
-if (end_seq < 0)
+if (end_seq == std::string::npos) {
   break;
-if (end_seq - (beg_seq + 2) < 0) {
+}
+if (end_seq < beg_seq + 2) {

Review comment:
   isn't this condition always false?





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


adamdebreceni commented on a change in pull request #964:
URL: https://github.com/apache/nifi-minifi-cpp/pull/964#discussion_r551891837



##
File path: libminifi/src/provenance/Provenance.cpp
##
@@ -189,8 +192,8 @@ bool 
ProvenanceEventRecord::Serialize(org::apache::nifi::minifi::io::BufferStrea
 return false;
   }
 }
-number = this->_childrenUuids.size();
-ret = outStream.write(number);
+uint32_t chldren_uuids_size = 
gsl::narrow(this->_childrenUuids.size());

Review comment:
   typo in variable name, missing `i` from `children`





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


adamdebreceni commented on a change in pull request #964:
URL: https://github.com/apache/nifi-minifi-cpp/pull/964#discussion_r551891610



##
File path: libminifi/src/provenance/Provenance.cpp
##
@@ -178,8 +181,8 @@ bool 
ProvenanceEventRecord::Serialize(org::apache::nifi::minifi::io::BufferStrea
 
   if (this->_eventType == ProvenanceEventRecord::FORK || this->_eventType == 
ProvenanceEventRecord::CLONE || this->_eventType == 
ProvenanceEventRecord::JOIN) {
 // write UUIDs
-uint32_t number = this->_parentUuids.size();
-ret = outStream.write(number);
+uint32_t parent_uuids_size = 
gsl::narrow(this->_parentUuids.size());

Review comment:
   an alternative name could be `parent_uuids_count`, although not sure if 
it is "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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org