[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1020: MINIFICPP-1352 Enable -Wall and -Wextra and resolve related warnings

[GitBox <gi...@apache.org>]

szaszm commented on a change in pull request #1020:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1020#discussion_r590417581



##########
File path: extensions/coap/controllerservice/CoapResponse.h
##########
@@ -73,7 +73,7 @@ class CoapResponse {
    * Returns a const pointer to the constant data.
    * @return data pointer.
    */
-  const uint8_t * const getData() const {
+  uint8_t* getData() const {

Review comment:
       Removing the left `const` is a change in behavior. Please revert.
   ```suggestion
     const uint8_t* getData() const {
   ```

##########
File path: cmake/Date.cmake
##########
@@ -69,6 +69,9 @@ if (NOT date_src_POPULATED)
     add_library(date::tz ALIAS date-tz)
     target_include_directories(date-tz PUBLIC ${DATE_INCLUDE_DIR})
     target_compile_features(date-tz PUBLIC cxx_std_11)
+    if (NOT WIN32)
+        target_compile_options(date-tz PRIVATE "-Wno-error")

Review comment:
       I feel like it would be cleaner to enable Werror on each of our own targets rather than disabling it where it's not used.

##########
File path: libminifi/test/unit/GeneralUtilsTest.cpp
##########
@@ -130,7 +130,7 @@ TEST_CASE("GeneralUtils::invoke FunctionObject", "[invoke function object]") {
   REQUIRE(false == utils::invoke(&free_function, false));
 
   const auto n = 3;
-  const auto int_timesn = [n](const int i) { return n * i; };
+  const auto int_timesn = [=](const int i) { return n * i; };

Review comment:
       I prefer the old version here.

##########
File path: nanofi/src/api/nanofi.cpp
##########
@@ -457,7 +457,7 @@ int get_content(const flow_file_record* ff, uint8_t* target, int size) {
     return stream->read(target, size);
   } else {
     file_buffer fb = file_to_buffer(ff->contentLocation);
-    size_t copy_size = size < fb.file_len ? size : gsl::narrow<size_t>(fb.file_len);
+    size_t copy_size = static_cast<size_t>(size) < fb.file_len ? size : gsl::narrow<size_t>(fb.file_len);

Review comment:
       I would check for negative `size` before casting to avoid surprises.

##########
File path: extensions/openwsman/processors/SourceInitiatedSubscriptionListener.cpp
##########
@@ -249,11 +249,11 @@ bool SourceInitiatedSubscriptionListener::loadState() {
 
 std::string SourceInitiatedSubscriptionListener::Handler::millisecondsToXsdDuration(int64_t milliseconds) {
   char buf[1024];
-  snprintf(buf, sizeof(buf), "PT%lld.%03lldS", milliseconds / 1000, milliseconds % 1000);
+  snprintf(buf, sizeof(buf), "PT%ld.%03ldS", milliseconds / 1000, milliseconds % 1000);

Review comment:
       Both the old and the new version are incorrect. The format specifier of `int64_t` is the `PRId64` macro from `<cinttypes>`.
   ```suggestion
     snprintf(buf, sizeof(buf), "PT%" PRId64 ".%03" PRId64 "S", milliseconds / 1000, milliseconds % 1000);
   ```

##########
File path: extensions/rocksdb-repos/RocksDatabase.h
##########
@@ -84,6 +84,7 @@ class RocksDatabase {
   };
 
   RocksDatabase(const rocksdb::Options& options, const std::string& name, Mode mode = Mode::ReadWrite);
+  virtual ~RocksDatabase() = default;

Review comment:
       Since there are no derived classes of `RocksDatabase`, I would rather make all members non-virtual.

##########
File path: bstrp_functions.sh
##########
@@ -375,6 +376,7 @@ show_supported_features() {
   echo "4. Use Shared Dependency Links .$(print_feature_status USE_SHARED_LIBS)"
   echo "5. Build Profile ...............$(print_multi_option_status BUILD_PROFILE)"
   echo "6. Create ASAN build ...........$(print_feature_status ASAN_ENABLED)"
+  echo "7. Treat warnings as errors.....$(print_feature_status FAIL_ON_WARNINGS)"

Review comment:
       The readme contains a reference bootstrap output. Could you update that?

##########
File path: extensions/script/python/PythonScriptEngine.cpp
##########
@@ -33,7 +33,7 @@ Interpreter *getInterpreter() {
 }
 
 PythonScriptEngine::PythonScriptEngine() {
-  auto intepreter = getInterpreter();
+  getInterpreter();

Review comment:
       There doesn't seem to be any magic global initialization happening here, so I suggest removing the calls altogether.

##########
File path: controller/Controller.h
##########
@@ -36,7 +36,7 @@ bool sendSingleCommand(std::unique_ptr<minifi::io::Socket> socket, uint8_t op, c
   minifi::io::BufferStream stream;
   stream.write(&op, 1);
   stream.write(value);
-  return socket->write(const_cast<uint8_t*>(stream.getBuffer()), gsl::narrow<int>(stream.size())) == stream.size();
+  return gsl::narrow<size_t>(socket->write(const_cast<uint8_t*>(stream.getBuffer()), gsl::narrow<int>(stream.size()))) == stream.size();

Review comment:
       `write` can return -1 so the narrowing conversion is unsafe. The comparison was safe before, it would simply return `false` in case of error.

##########
File path: extensions/script/lua/LuaBaseStream.cpp
##########
@@ -54,15 +54,15 @@ std::string LuaBaseStream::read(size_t len) {
   // http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf
   auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), static_cast<int>(len));
 
-  if (read != len) {
-    buffer.resize(static_cast<size_t >(read));
+  if (gsl::narrow<size_t>(read) != len) {
+    buffer.resize(gsl::narrow<size_t>(read));
   }
 
-  return std::move(buffer);
+  return buffer;
 }
 
 size_t LuaBaseStream::write(std::string buf) {
-  return static_cast<size_t>(stream_->write(reinterpret_cast<uint8_t *>(const_cast<char *>(buf.data())),
+  return gsl::narrow<size_t>(stream_->write(reinterpret_cast<uint8_t *>(const_cast<char *>(buf.data())),

Review comment:
       same here

##########
File path: extensions/script/lua/LuaBaseStream.cpp
##########
@@ -54,15 +54,15 @@ std::string LuaBaseStream::read(size_t len) {
   // http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf
   auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), static_cast<int>(len));
 
-  if (read != len) {
-    buffer.resize(static_cast<size_t >(read));
+  if (gsl::narrow<size_t>(read) != len) {
+    buffer.resize(gsl::narrow<size_t>(read));

Review comment:
       Unsafe narrowing: `read` can be `-1` in case of an error.

##########
File path: extensions/standard-processors/processors/ListenSyslog.cpp
##########
@@ -204,7 +204,7 @@ void ListenSyslog::runThread() {
           logger_->log_debug("ListenSysLog client socket %d close", clientSocket);
           it = _clientSockets.erase(it);
         } else {
-          if ((uint64_t) (recvlen + getEventQueueByteSize()) <= _recvBufSize) {
+          if ((uint64_t) (recvlen + getEventQueueByteSize()) <= gsl::narrow<uint64_t>(_recvBufSize)) {

Review comment:
       same as above

##########
File path: libminifi/include/utils/Monitors.h
##########
@@ -41,7 +41,7 @@ class AfterExecute {
 
   AfterExecute() = default;
 
-  explicit AfterExecute(AfterExecute &&other) {
+  explicit AfterExecute(AfterExecute&& /*other*/) {

Review comment:
       Can we remove `explicit` and explicitly `default` this?

##########
File path: extensions/standard-processors/processors/ListenSyslog.h
##########
@@ -191,7 +191,7 @@ class ListenSyslog : public core::Processor {
   void pollEvent(std::queue<SysLogEvent> &list, int maxSize) {
     std::lock_guard<std::mutex> lock(mutex_);
 
-    while (!_eventQueue.empty() && (maxSize == 0 || list.size() < maxSize)) {
+    while (!_eventQueue.empty() && (maxSize == 0 || list.size() < gsl::narrow<uint32_t>(maxSize))) {

Review comment:
       same as above

##########
File path: extensions/standard-processors/processors/ListenSyslog.cpp
##########
@@ -187,7 +187,7 @@ void ListenSyslog::runThread() {
         struct sockaddr_in cli_addr;
         clilen = sizeof(cli_addr);
         int recvlen = recvfrom(_serverSocket, _buffer, sizeof(_buffer), 0, (struct sockaddr *) &cli_addr, &clilen);
-        if (recvlen > 0 && (uint64_t) (recvlen + getEventQueueByteSize()) <= _recvBufSize) {
+        if (recvlen > 0 && (uint64_t) (recvlen + getEventQueueByteSize()) <= gsl::narrow<uint64_t>(_recvBufSize)) {

Review comment:
       Unsafe narrowing. `_recvBufSize` is parsed from a property, meaning it has to be validated. `gsl::narrow` should be used when you "promise" the value will fit in the destination type, or otherwise you're willing to crash the program.




----------------------------------------------------------------
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:
users@infra.apache.org