Skip to content

MINIFI-286: Fix compilation error when GCC version is greater than 4.9#88

Closed
jdye64 wants to merge 3 commits intoapache:masterfrom
jdye64:MINIFI-286
Closed

MINIFI-286: Fix compilation error when GCC version is greater than 4.9#88
jdye64 wants to merge 3 commits intoapache:masterfrom
jdye64:MINIFI-286

Conversation

@jdye64
Copy link
Contributor

@jdye64 jdye64 commented May 2, 2017

Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced
    in the commit message?

  • Does your PR title start with MINIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

  • Has your PR been rebased against the latest commit within the target branch (typically master)?

  • Is your initial contribution a single, squashed commit?

For code changes:

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file?
  • If applicable, have you updated the NOTICE file?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

@jdye64 jdye64 changed the title MINIFI-286: Fix compilcation error when GCC version is greater than 4.9 MINIFI-286: Fix compilation error when GCC version is greater than 4.9 May 2, 2017
@apiri
Copy link
Member

apiri commented May 3, 2017

reviewing

@apiri
Copy link
Member

apiri commented May 3, 2017

Hey @jdye64,

I was not able to make the current codebase fail under Ubuntu 17.04 which has gcc 6.3.0 (12ubuntu2). Any other considerations of your environment?

@phrocker
Copy link
Contributor

phrocker commented May 3, 2017

@apiri The logic can be improved @jdye64 I'm in favor of removing the conditional expression entirely and using the c style for all versions since that will work. There's no point in using the C++ regex matching since it won't be available for all builds. If you really want to maintain it, you can do this:

diff --git a/libminifi/src/processors/GetFile.cpp b/libminifi/src/processors/GetFile.cpp
index 2740793..9988029 100644
--- a/libminifi/src/processors/GetFile.cpp
+++ b/libminifi/src/processors/GetFile.cpp
@@ -24,8 +24,9 @@
 #include <dirent.h>
 #include <limits.h>
 #include <unistd.h>
-#if  (__GNUC__ >= 4)
-#if (__GNUC_MINOR__ < 9)
+// if an older version of gcc that doesn't support <regex>
+#ifdef __GNUC__
+#if (__GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 9))
 #include <regex.h>
 #else
 #include <regex>
@@ -268,8 +269,7 @@ bool GetFile::acceptFile(std::string fullName, std::string name,
       return false;
 
 #ifdef __GNUC__
-#if (__GNUC__ >= 4)
-#if (__GNUC_MINOR__ < 9)
+#if (__GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 9))
     regex_t regex;
     int ret = regcomp(&regex, request.fileFilter.c_str(), 0);
     if (ret)
@@ -280,7 +280,7 @@ bool GetFile::acceptFile(std::string fullName, std::string name,
       return false;
 #else
     try {
-      std::regex re(fileFilter);
+      std::regex re(request.fileFilter);
 
       if (!std::regex_match(name, re)) {
         return false;
@@ -290,7 +290,6 @@ bool GetFile::acceptFile(std::string fullName, std::string name,
       return false;
     }
 #endif
-#endif
 #else
     logger_->log_info("Cannot support regex filtering");
 #endif

The alternative, would be something like this:

diff --git a/libminifi/src/processors/GetFile.cpp b/libminifi/src/processors/GetFile.cpp
index 2740793..fcd601f 100644
--- a/libminifi/src/processors/GetFile.cpp
+++ b/libminifi/src/processors/GetFile.cpp
@@ -16,6 +16,7 @@
  * limitations under the License.
  */
 #include "processors/GetFile.h"
+#include <regex.h>
 #include <sys/time.h>
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -24,13 +25,6 @@
 #include <dirent.h>
 #include <limits.h>
 #include <unistd.h>
-#if  (__GNUC__ >= 4)
-#if (__GNUC_MINOR__ < 9)
-#include <regex.h>
-#else
-#include <regex>
-#endif
-#endif
 #include <vector>
 #include <queue>
 #include <map>
@@ -267,9 +261,6 @@ bool GetFile::acceptFile(std::string fullName, std::string name,
     if (request.keepSourceFile == false && access(fullName.c_str(), W_OK) != 0)
       return false;
 
-#ifdef __GNUC__
-#if (__GNUC__ >= 4)
-#if (__GNUC_MINOR__ < 9)
     regex_t regex;
     int ret = regcomp(&regex, request.fileFilter.c_str(), 0);
     if (ret)
@@ -278,22 +269,7 @@ bool GetFile::acceptFile(std::string fullName, std::string name,
     regfree(&regex);
     if (ret)
       return false;
-#else
-    try {
-      std::regex re(fileFilter);
 
-      if (!std::regex_match(name, re)) {
-        return false;
-      }
-    } catch (std::regex_error e) {
-      logger_->log_error("Invalid File Filter regex: %s.", e.what());
-      return false;
-    }
-#endif
-#endif
-#else
-    logger_->log_info("Cannot support regex filtering");
-#endif

@jdye64
Copy link
Contributor Author

jdye64 commented May 3, 2017

I like just removing the conditional expression. I'm going to update to that now.

@apiri
Copy link
Member

apiri commented May 3, 2017

cool, looks good and was able to verify the issue beforehand with a specific docker environment that hit the bad logic. I did notice there was a linter issue but I can take care of that on merge

Thanks for correcting this!

@asfgit asfgit closed this in 78d5277 May 3, 2017
nghiaxlee pushed a commit to nghiaxlee/nifi-minifi-cpp that referenced this pull request Jul 8, 2019
This closes apache#88.

Signed-off-by: Aldrin Piri <aldrin@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants