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

2021-01-12 Thread GitBox 


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



##
File path: extensions/rocksdb-repos/DatabaseContentRepository.cpp
##
@@ -94,7 +97,7 @@ void DatabaseContentRepository::Session::commit() {
 if (outStream == nullptr) {
   throw Exception(REPOSITORY_EXCEPTION, "Couldn't open the underlying 
resource for append: " + resource.first->getContentFullPath());
 }
-const auto size = resource.second->size();
+const int size = gsl::narrow(resource.second->size());

Review comment:
   @arpadboda was quicker than you :)
   
   yes, this would have been an improvement, but I don't think it's essential, 
so let's leave it as it is, for now





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] fgerlits commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-11 Thread GitBox 


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



##
File path: libminifi/include/utils/StringUtils.h
##
@@ -159,9 +160,9 @@ class StringUtils {
   }
 
   inline static std::string hex_ascii(const std::string& in) {
-int len = in.length();
+size_t len = in.length();

Review comment:
   I have inlined the variable, and added a `reserve` (though I don't think 
this will increase performance by a measurable amount).





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] fgerlits commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-11 Thread GitBox 


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



##
File path: libminifi/include/provenance/Provenance.h
##
@@ -360,7 +361,7 @@ class ProvenanceEventRecord : public 
core::SerializableComponent {
   bool DeSerialize(const std::shared_ptr );
 
   uint64_t getEventTime(const uint8_t *buffer, const size_t bufferSize) {
-int size = bufferSize > 72 ? 72 : bufferSize;
+int size = gsl::narrow(bufferSize > 72 ? 72 : bufferSize);

Review comment:
   true -- I've replaced it with `min`





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] fgerlits commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-11 Thread GitBox 


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



##
File path: extensions/rocksdb-repos/FlowFileRepository.cpp
##
@@ -15,12 +15,11 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-#include "FlowFileRecord.h"
 #include "FlowFileRepository.h"
 
-#include "rocksdb/options.h"
-#include "rocksdb/write_batch.h"
-#include "rocksdb/slice.h"
+#include 
+#include 

Review comment:
   fixed





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] fgerlits commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-06 Thread GitBox 


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



##
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:
   Good point.  I have removed 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.

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

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

2021-01-05 Thread GitBox 


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



##
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:
   This is nanofi code, which is supposed to be in C.  This is not entirely 
true, as there is some C++ code in it already, but I did not want to add more.





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] fgerlits commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


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



##
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:
   Good catch, thanks!  I have changed the type of `i` back to `int`.





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] fgerlits commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


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



##
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:
   I've removed it

##
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:
   fixed

##
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:
   yes





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] fgerlits commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


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



##
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:
   I think we can assume that `string::size_type` will always be `size_t`, 
but you are right, `string::size_type` is more correct, so I changed 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.

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

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

2021-01-05 Thread GitBox 


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



##
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:
   Yes.  I think the intention was to check for `<=`, as a `"... ${} ..."` 
input causes an infinite loop without this check.  I have changed it to `<=` 
and added some more unit tests.  (Which showed another bug, which I also fixed.)





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] fgerlits commented on a change in pull request #964: MINIFICPP-1443 Fix 'possible loss of data' warnings

2021-01-05 Thread GitBox 


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



##
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:
   yes, I think it's slightly better -- changed it to `..._count`

##
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:
   fixed





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