[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #809: MINIFICPP-1254 - Introduce optional-lite

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

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



##########
File path: CMakeLists.txt
##########
@@ -307,6 +307,9 @@ if (STRICT_GSL_CHECKS STREQUAL "DEBUG_ONLY")
 endif()
 target_compile_definitions(gsl-lite INTERFACE ${GslDefinitions})
 
+# optional-lite
+add_library(optional-lite INTERFACE)
+target_include_directories(optional-lite INTERFACE "${CMAKE_CURRENT_SOURCE_DIR}/thirdparty/optional-lite/include")

Review comment:
       Please use a released version and include the version in the thirdparty directory name. When you download and extract a release from github, it contains the version in the extracted directory name.

##########
File path: libminifi/src/utils/BackTrace.cpp
##########
@@ -89,19 +90,9 @@ void pull_trace(uint8_t frames_to_skip /* = 1 */) {
       }
     }
 
-    /* Determine the symbol name */
-    std::string demangled_symbol_name;
+    std::string symbol_name;
     if (dl_info.dli_sname != nullptr) {
-      symbol_name = dl_info.dli_sname;
-
-      /* Try to demangle the symbol name */
-      demangled_symbol_name = demangle_symbol(symbol_name);
-      if (!demangled_symbol_name.empty()) {
-        symbol_name = demangled_symbol_name.c_str();
-      }
-    } else {
-      /* If we could not determine the symbol name, we will use the filename instead */
-      symbol_name = file_name;
+      symbol_name = demangle_symbol(dl_info.dli_sname).value_or(std::string(file_name));
     }

Review comment:
       The new code is not equivalent. When `dl_info.dli_sname == nullptr`, then we leave the `symbol_name` empty instead of falling back to the file name, and when `demangle_symbol` returns an empty string, not we do fall back to the file name. The latter is fine, but the former is not IMO.
   
   With an optional implementation that includes the monadic operations, this could be a possible implementation:
   ```
   const std::string demangled_symbol_name = optional_from_nullable(dl_info.dli_sname)
       .and_then(demangle_symbol)
       .value_or(file_name);
   ```
   
   or with the current version:
   ```
   const std::string demangled_symbol_name = !dl_info.dli_sname 
       ? file_name 
       : demangle_symbol(dl_info.dli_sname)
           .value_or(file_name);

##########
File path: libminifi/src/utils/BackTrace.cpp
##########
@@ -30,22 +30,24 @@
 #include <utility>
 #endif
 
+#include <nonstd/optional.hpp>

Review comment:
       Because the this optional implementation (as well as the c++17 `std::optional`) is quite limited, I recommend introducing our own header and alias that we can customize later if desired.
   
   A first extension idea from my side would be adding an `optional_from_nullable` function that behaves as the name suggests.
   ```
   template<typename T>
   optional<typename std::remove_cvref<T>::type> optional_from_nullable(T&& obj) {
     return obj == nullptr ? {} : optional<typename std::remove_cvref<T>::type>{ std::forward<T>(obj) };
   }
   ```




----------------------------------------------------------------
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