[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1133: MINIFICPP-1425 upgrade compilers on linux and mac, use C++17, prepare for C++20

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

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



##########
File path: extensions/expression-language/Expression.cpp
##########
@@ -25,6 +25,10 @@
 #include <functional>
 #include <string>
 
+#ifdef GetObject
+#undef GetObject
+#endif

Review comment:
       As in #1129, I would change all occurrences of this to:
   ```suggestion
   #undef GetObject  // windows.h #defines GetObject = GetObjectA or GetObjectW, which conflicts with rapidjson
   ```
   
   (not sure if they are the same lines; if so then I guess resolving merge conflicts will take care of this)

##########
File path: libminifi/test/Utils.h
##########
@@ -16,8 +16,16 @@
  */
 #pragma once
 
+#ifdef GetObject
+#undef GetObject
+#endif
+
 #include <string>
 #include <utility>
+
+#ifdef GetObject
+#undef GetObject
+#endif

Review comment:
       do we need this twice?

##########
File path: extensions/script/CMakeLists.txt
##########
@@ -37,18 +37,20 @@ if (NOT DISABLE_PYTHON_SCRIPTING)
     	find_package(PythonLibs 3.0 REQUIRED)
     endif()
 
-    include_directories(${PYTHON_INCLUDE_DIR})
-    include_directories(SYSTEM ../../thirdparty/pybind11/include)
-
-    include_directories(python)
     add_definitions(-DPYTHON_SUPPORT)
 
     file(GLOB PY_SOURCES  "python/*.cpp")
     add_library(minifi-python-extensions STATIC ${PY_SOURCES})
-
+    target_include_directories(minifi-python-extensions SYSTEM PRIVATE ${PYTHON_INCLUDE_DIR})
+    target_include_directories(minifi-python-extensions SYSTEM PRIVATE ../../thirdparty/pybind11/include)
+    target_include_directories(minifi-python-extensions PRIVATE python)
     target_link_libraries(minifi-python-extensions ${LIBMINIFI})
     target_link_libraries(minifi-python-extensions ${PYTHON_LIBRARIES})
+    target_compile_options(minifi-python-extensions PUBLIC $<$<CXX_COMPILER_ID:Clang>:-fsized-deallocation>)

Review comment:
       just out of curiosity, why is this needed?

##########
File path: linux.sh
##########
@@ -16,42 +16,6 @@
 # specific language governing permissions and limitations
 # under the License.
 verify_gcc_enable(){
-  feature="$1"
-  if [ "$feature" = "BUSTACHE_ENABLED" ]; then
-    if (( COMPILER_MAJOR == 6 && COMPILER_MINOR >= 3 && COMPILER_REVISION >= 1  )); then
-      echo "true"
-    elif (( COMPILER_MAJOR > 6 )); then
-      echo "true"
-    else
-      echo "false"
-    fi
-  elif [ "$feature" = "EXECUTE_SCRIPT_ENABLED" ]; then
-    if (( COMPILER_MAJOR >= 6 )); then
-      echo "true"
-    else
-      echo "false"
-    fi
-  elif [ "$feature" = "KAFKA_ENABLED" ]; then
-    if (( COMPILER_MAJOR >= 4 )); then
-      if (( COMPILER_MAJOR > 4 || COMPILER_MINOR >= 8 )); then
-        echo "true"
-      else
-        echo "false"
-      fi
-    else
-      echo "false"
-    fi
-   elif [ "$feature" = "PCAP_ENABLED" ]; then
-    if (( COMPILER_MAJOR >= 4 )); then
-      if (( COMPILER_MAJOR > 4 || COMPILER_MINOR >= 8 )); then
-        echo "true"
-      else
-        echo "false"
-      fi
-    else
-      echo "false"
-    fi
-  else
-    echo "true"
-  fi
+  #feature="$1"

Review comment:
       can this line be removed?

##########
File path: README.md
##########
@@ -154,45 +147,22 @@ and rebuild.
 * libgps-dev -- Required if building libGPS support
 * Zlib headers
 
-#### CentOS 6
+#### CentOS 7
 
-Additional environmental preparations are required for CentOS 6 support. Before
+Additional environmental preparations are required for CentOS 7 support. Before
 building, install and enable the devtoolset-6 SCL:

Review comment:
       ```suggestion
   building, install and enable the devtoolset-10 SCL:
   ```




-- 
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: issues-unsubscribe@nifi.apache.org

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