PengZheng commented on code in PR #692:
URL: https://github.com/apache/celix/pull/692#discussion_r1420025493


##########
libs/utils/src/filter.c:
##########
@@ -17,421 +17,491 @@
  * under the License.
  */
 
+#include <assert.h>
+#include <ctype.h>
 #include <stdio.h>
-#include <string.h>
 #include <stdlib.h>
-#include <ctype.h>
-#include <assert.h>
+#include <string.h>
 #include <utils.h>
 
-#include "celix_filter.h"
-#include "filter.h"
+#include "celix_convert_utils.h"
+#include "celix_err.h"
 #include "celix_errno.h"
+#include "celix_filter.h"
+#include "celix_stdio_cleanup.h"
+#include "celix_stdlib_cleanup.h"
 #include "celix_version.h"
-#include "celix_convert_utils.h"
+#include "filter.h"
+
+// ignoring clang-tidy recursion warnings for this file, because filter uses 
recursion
+// NOLINTBEGIN(misc-no-recursion)
 
 struct celix_filter_internal {
     bool convertedToLong;
     long longValue;
+
     bool convertedToDouble;
     double doubleValue;
+
+    bool convertedToBool;
+    bool boolValue;
+
     bool convertedToVersion;
-    celix_version_t *versionValue;
+    celix_version_t* versionValue;
 };
 
-static void filter_skipWhiteSpace(char* filterString, int* pos);
-static celix_filter_t * filter_parseFilter(char* filterString, int* pos);
-static celix_filter_t * filter_parseFilterComp(char* filterString, int* pos);
-static celix_filter_t * filter_parseAndOrOr(char* filterString, 
celix_filter_operand_t andOrOr, int* pos);
-static celix_filter_t * filter_parseNot(char* filterString, int* pos);
-static celix_filter_t * filter_parseItem(char* filterString, int* pos);
-static char * filter_parseAttr(char* filterString, int* pos);
-static char * filter_parseValue(char* filterString, int* pos);
-static celix_array_list_t* filter_parseSubstring(char* filterString, int* pos);
-
-static celix_status_t filter_compare(const celix_filter_t* filter, const char 
*propertyValue, bool *result);
-
-static void filter_skipWhiteSpace(char * filterString, int * pos) {
-    int length;
-    for (length = strlen(filterString); (*pos < length) && 
isspace(filterString[*pos]);) {
+static void celix_filter_skipWhiteSpace(const char* filterString, int* pos);
+static celix_filter_t* celix_filter_parseFilter(const char* filterString, int* 
pos);
+static celix_filter_t* celix_filter_parseFilterNode(const char* filterString, 
int* pos);
+static celix_filter_t* celix_filter_parseAndOrOr(const char* filterString, 
celix_filter_operand_t andOrOr, int* pos);
+static celix_filter_t* celix_filter_parseNot(const char* filterString, int* 
pos);
+static celix_filter_t* celix_filter_parseItem(const char* filterString, int* 
pos);
+static char* celix_filter_parseAttributeOrValue(const char* filterString, int* 
pos, bool parseAttribute);
+static celix_array_list_t* celix_filter_parseSubstring(const char* 
filterString, int* pos);
+static bool celix_filter_isSubString(const char* filterString, int startPos);
+static bool celix_filter_isNextNonWhiteSpaceChar(const char* filterString, int 
pos, char c);
+
+static void celix_filter_skipWhiteSpace(const char* filterString, int* pos) {
+    for (; filterString[*pos] != '\0' && isspace(filterString[*pos]);) {
         (*pos)++;
     }
 }
 
-celix_filter_t * filter_create(const char* filterString) {
-    return celix_filter_create(filterString);
+static bool celix_filter_isNextNonWhiteSpaceChar(const char* filterString, int 
pos, char c) {
+    for (int i = pos; filterString[i] != '\0'; i++) {
+        if (!isspace(filterString[i])) {
+            return filterString[i] == c;
+        }
+    }
+    return false;
 }
 
-void filter_destroy(celix_filter_t * filter) {
-    return celix_filter_destroy(filter);
-}
+celix_filter_t* filter_create(const char* filterString) { return 
celix_filter_create(filterString); }
 
-static celix_filter_t * filter_parseFilter(char* filterString, int* pos) {
-    celix_filter_t * filter;
-    filter_skipWhiteSpace(filterString, pos);
-    if (filterString[*pos] == '\0') {
-        //empty filter
-        fprintf(stderr, "Filter Error: Cannot create LDAP filter from an empty 
string.\n");
-        return NULL;
-    } else if (filterString[*pos] != '(') {
-        fprintf(stderr, "Filter Error: Missing '(' in filter string '%s'.\n", 
filterString);
+void filter_destroy(celix_filter_t* filter) { return 
celix_filter_destroy(filter); }
+
+static celix_filter_t* celix_filter_parseFilter(const char* filterString, int* 
pos) {
+    celix_filter_skipWhiteSpace(filterString, pos);
+    if (filterString[*pos] != '(') {
+        celix_err_pushf("Filter Error: Missing '(' in filter string '%s'.", 
filterString);
         return NULL;
     }
-    (*pos)++; //eat '('
+    (*pos)++; // eat '('
 
-    filter = filter_parseFilterComp(filterString, pos);
+    celix_autoptr(celix_filter_t) filter = 
celix_filter_parseFilterNode(filterString, pos);
+    if (!filter) {
+        return NULL;
+    }
 
-    filter_skipWhiteSpace(filterString, pos);
+    celix_filter_skipWhiteSpace(filterString, pos);
 
     if (filterString[*pos] != ')') {
-        fprintf(stderr, "Filter Error: Missing ')' in filter string '%s'.\n", 
filterString);
-        if(filter!=NULL){
-            filter_destroy(filter);
-        }
+        celix_err_pushf("Filter Error: Missing ')' in filter string '%s'.", 
filterString);
         return NULL;
     }
-    (*pos)++; //eat ')'
-    filter_skipWhiteSpace(filterString, pos);
+    (*pos)++; // eat ')'
+    celix_filter_skipWhiteSpace(filterString, pos);
 
-    return filter;
+    return celix_steal_ptr(filter);
 }
 
-static celix_filter_t * filter_parseFilterComp(char * filterString, int * pos) 
{
+static celix_filter_t* celix_filter_parseFilterNode(const char* filterString, 
int* pos) {
     char c;
-    filter_skipWhiteSpace(filterString, pos);
+    celix_filter_skipWhiteSpace(filterString, pos);
 
     c = filterString[*pos];
 
     switch (c) {
-        case '&': {
-            (*pos)++;
-            return filter_parseAndOrOr(filterString, CELIX_FILTER_OPERAND_AND, 
pos);
-        }
-        case '|': {
-            (*pos)++;
-            return filter_parseAndOrOr(filterString, CELIX_FILTER_OPERAND_OR, 
pos);
-        }
-        case '!': {
-            (*pos)++;
-            return filter_parseNot(filterString, pos);
-        }
+    case '&': {
+        (*pos)++;
+        return celix_filter_parseAndOrOr(filterString, 
CELIX_FILTER_OPERAND_AND, pos);
+    }
+    case '|': {
+        (*pos)++;
+        return celix_filter_parseAndOrOr(filterString, 
CELIX_FILTER_OPERAND_OR, pos);
+    }
+    case '!': {
+        (*pos)++;
+        return celix_filter_parseNot(filterString, pos);
+    }
+    default: {
+        return celix_filter_parseItem(filterString, pos);
+    }
     }
-    return filter_parseItem(filterString, pos);
 }
 
-static celix_filter_t * filter_parseAndOrOr(char * filterString, 
celix_filter_operand_t andOrOr, int * pos) {
-
-    filter_skipWhiteSpace(filterString, pos);
-    bool failure = false;
+static celix_filter_t* celix_filter_parseAndOrOr(const char* filterString, 
celix_filter_operand_t andOrOr, int* pos) {
+    celix_filter_skipWhiteSpace(filterString, pos);
 
-    if (filterString[*pos] != '(') {
-        fprintf(stderr, "Filter Error: Missing '('.\n");
+    celix_autoptr(celix_filter_t) filter = (celix_filter_t*)calloc(1, 
sizeof(*filter));
+    if (!filter) {
+        celix_err_push("Filter Error: Failed to allocate memory.");
         return NULL;
     }
 
-    celix_array_list_t *children = celix_arrayList_create();
-    while(filterString[*pos] == '(') {
-        celix_filter_t * child = filter_parseFilter(filterString, pos);
-        if(child == NULL) {
-            failure = true;
-            break;
-        }
-        celix_arrayList_add(children, child);
+    celix_array_list_create_options_t ops = 
CELIX_EMPTY_ARRAY_LIST_CREATE_OPTIONS;
+    ops.simpleRemovedCallback = (void (*)(void*))celix_filter_destroy;
+    celix_autoptr(celix_array_list_t) children = 
celix_arrayList_createWithOptions(&ops);
+    if (!children) {
+        celix_err_push("Filter Error: Failed to allocate memory.");
+        return NULL;
     }
 
-    if(failure == true){
-        for (int i = 0; i < celix_arrayList_size(children); ++i) {
-            celix_filter_t * f = celix_arrayList_get(children, i);
-            filter_destroy(f);
-        }
-        celix_arrayList_destroy(children);
+    if (filterString[*pos] == ')') {
+        // empty and/or filter
+    } else if (filterString[*pos] != '(') {
+        celix_err_push("Filter Error: Missing '('.");
         return NULL;
+    } else {
+        while (filterString[*pos] == '(') {
+            celix_filter_t* child = celix_filter_parseFilter(filterString, 
pos);
+            if (child == NULL) {
+                return NULL;
+            }
+            celix_arrayList_add(children, child);
+        }
     }
 
-    celix_filter_t * filter = (celix_filter_t *) calloc(1, sizeof(*filter));
     filter->operand = andOrOr;
-    filter->children = children;
+    filter->children = celix_steal_ptr(children);
 
-    return filter;
+    return celix_steal_ptr(filter);
 }
 
-static celix_filter_t * filter_parseNot(char * filterString, int * pos) {
-    celix_filter_t * child = NULL;
-    filter_skipWhiteSpace(filterString, pos);
+static celix_filter_t* celix_filter_parseNot(const char* filterString, int* 
pos) {
+    celix_filter_skipWhiteSpace(filterString, pos);
 
-    if (filterString[*pos] != '(') {
-        fprintf(stderr, "Filter Error: Missing '('.\n");
+    char c = filterString[*pos];
+    if (c != '(') {
+        celix_err_push("Filter Error: Missing '('.");
+        return NULL;
+    }
+
+    celix_filter_t* child = celix_filter_parseFilter(filterString, pos);
+    if (!child) {
         return NULL;
     }
 
-    child = filter_parseFilter(filterString, pos);
-    if (child == NULL) {
+    celix_array_list_create_options_t ops = 
CELIX_EMPTY_ARRAY_LIST_CREATE_OPTIONS;
+    ops.simpleRemovedCallback = (void (*)(void*))celix_filter_destroy;
+    celix_array_list_t* children = celix_arrayList_createWithOptions(&ops);
+    if (!children) {
+        celix_err_push("Filter Error: Failed to allocate memory.");
+        celix_filter_destroy(child);
         return NULL;
     }
-    celix_array_list_t* children = celix_arrayList_create();
     celix_arrayList_add(children, child);
 
-    celix_filter_t * filter = (celix_filter_t *) calloc(1, sizeof(*filter));
-    filter->operand = CELIX_FILTER_OPERAND_NOT;
-    filter->children = children;
+    celix_filter_t* filter = (celix_filter_t*)calloc(1, sizeof(*filter));
+    if (!filter) {
+        celix_err_push("Filter Error: Failed to allocate memory.");
+        celix_arrayList_destroy(children);
+        return NULL;
+    }
 
+    filter->operand = CELIX_FILTER_OPERAND_NOT;
+    filter->children = children; //note can be NULL

Review Comment:
   The comment contradicts with the previous error handling.



-- 
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: dev-unsubscr...@celix.apache.org

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

Reply via email to