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


##########
libs/utils/src/filter.c:
##########
@@ -463,367 +564,345 @@ static celix_status_t 
celix_filter_compile(celix_filter_t* filter) {
     return CELIX_SUCCESS;
 }
 
-celix_status_t filter_match(celix_filter_t * filter, celix_properties_t 
*properties, bool *out) {
+celix_status_t filter_match(celix_filter_t* filter, celix_properties_t* 
properties, bool* out) {
     bool result = celix_filter_match(filter, properties);
     if (out != NULL) {
         *out = result;
     }
     return CELIX_SUCCESS;
 }
 
-static int celix_filter_compareAttributeValue(const celix_filter_t* filter, 
const char* propertyValue) {
-    if (!filter->internal->convertedToLong && 
!filter->internal->convertedToDouble && !filter->internal->convertedToVersion) {
-        return strcmp(propertyValue, filter->value);
+static int celix_filter_cmpLong(long a, long b) {
+    if (a < b) {
+        return -1;
+    } else if (a > b) {
+        return 1;
+    } else {
+        return 0;
     }
+}
 
-    if (filter->internal->convertedToLong) {
-        bool propertyValueIsLong = false;
-        long value = celix_utils_convertStringToLong(propertyValue, 0, 
&propertyValueIsLong);
-        if (propertyValueIsLong) {
-            if (value < filter->internal->longValue)
-                return -1;
-            else if (value > filter->internal->longValue)
-                return 1;
-            else
-                return 0;
-        }
-    }
-
-    if (filter->internal->convertedToDouble) {
-        bool propertyValueIsDouble = false;
-        double value = celix_utils_convertStringToDouble(propertyValue, 0.0, 
&propertyValueIsDouble);
-        if (propertyValueIsDouble) {
-            if (value < filter->internal->doubleValue) {
-                return -1;
-            } else if (value > filter->internal->doubleValue) {
-                return 1;
-            } else {
-                return 0;
-            }
-        }
+static int celix_filter_cmpDouble(double a, double b) {
+    if (a < b) {
+        return -1;
+    } else if (a > b) {
+        return 1;
+    } else {
+        return 0;
     }
+}
 
-    if (filter->internal->convertedToVersion) {
-        bool propertyValueIsVersion = false;
-        celix_version_t *value = 
celix_utils_convertStringToVersion(propertyValue, NULL, 
&propertyValueIsVersion);
-        if (propertyValueIsVersion) {
-            int cmp = celix_version_compareTo(value, 
filter->internal->versionValue);
-            celix_version_destroy(value);
+static int celix_filter_cmpBool(bool a, bool b) {
+    if (a == b) {
+        return 0;
+    } else if (a) {
+        return 1;
+    } else {
+        return -1;
+    }
+}
+
+static int celix_filter_compareAttributeValue(const celix_filter_t* filter, 
const celix_properties_entry_t* entry) {
+    // not converted, fallback on string compare
+    if (!filter->internal->convertedToLong && 
!filter->internal->convertedToDouble &&
+        !filter->internal->convertedToBool && 
!filter->internal->convertedToVersion) {
+        return strcmp(entry->value, filter->value);
+    }
+
+    // compare typed values
+    if (filter->internal->convertedToLong && entry->valueType == 
CELIX_PROPERTIES_VALUE_TYPE_LONG) {
+        return celix_filter_cmpLong(entry->typed.longValue, 
filter->internal->longValue);
+    } else if (filter->internal->convertedToDouble && entry->valueType == 
CELIX_PROPERTIES_VALUE_TYPE_DOUBLE) {
+        return celix_filter_cmpDouble(entry->typed.doubleValue, 
filter->internal->doubleValue);
+    } else if (filter->internal->convertedToBool && entry->valueType == 
CELIX_PROPERTIES_VALUE_TYPE_BOOL) {
+        return celix_filter_cmpBool(entry->typed.boolValue, 
filter->internal->boolValue);
+    } else if (filter->internal->convertedToVersion && entry->valueType == 
CELIX_PROPERTIES_VALUE_TYPE_VERSION) {
+        return celix_version_compareTo(entry->typed.versionValue, 
filter->internal->versionValue);
+    }
+
+    // check if the property string value can be converted to the filter value 
type
+    bool propertyConverted;
+    if (filter->internal->convertedToLong) {
+        long val = celix_utils_convertStringToLong(entry->value, 0, 
&propertyConverted);
+        if (propertyConverted) {
+            return celix_filter_cmpLong(val, filter->internal->longValue);
+        }
+    } else if (filter->internal->convertedToDouble) {
+        double val = celix_utils_convertStringToDouble(entry->value, 0.0, 
&propertyConverted);
+        if (propertyConverted) {
+            return celix_filter_cmpDouble(val, filter->internal->doubleValue);
+        }
+    } else if (filter->internal->convertedToBool) {
+        bool val = celix_utils_convertStringToBool(entry->value, false, 
&propertyConverted);
+        if (propertyConverted) {
+            return celix_filter_cmpBool(val, filter->internal->boolValue);
+        }
+    } else if (filter->internal->convertedToVersion) {
+        celix_version_t* val = 
celix_utils_convertStringToVersion(entry->value, NULL, &propertyConverted);
+        if (propertyConverted) {
+            int cmp = celix_version_compareTo(val, 
filter->internal->versionValue);
+            celix_version_destroy(val);
             return cmp;
         }
     }
 
-    //fallback on string compare
-    return strcmp(propertyValue, filter->value);
+    // fallback on string compare
+    return strcmp(entry->value, filter->value);
 }
 
-static celix_status_t filter_compare(const celix_filter_t* filter, const char 
*propertyValue, bool *out) {
-    celix_status_t  status = CELIX_SUCCESS;
-    bool result = false;
+static bool celix_filter_matchSubString(const celix_filter_t* filter, const 
celix_properties_entry_t* entry) {
+    assert(filter->children && celix_arrayList_size(filter->children) >= 2);
 
-    if (filter == NULL || propertyValue == NULL) {
-        *out = false;
-        return status;
-    }
+    size_t strLen = celix_utils_strlen(entry->value);
+    const char* initial = celix_arrayList_get(filter->children, 0);
+    const char* final = celix_arrayList_get(filter->children, 
celix_arrayList_size(filter->children) - 1);
 
-    switch (filter->operand) {
-        case CELIX_FILTER_OPERAND_SUBSTRING: {
-            int pos = 0;
-            int size = celix_arrayList_size(filter->children);
-            for (int i = 0; i < size; i++) {
-                char * substr = (char *) celix_arrayList_get(filter->children, 
i);
-
-                if (i + 1 < size) {
-                    if (substr == NULL) {
-                        unsigned int index;
-                        char * substr2 = (char *) 
celix_arrayList_get(filter->children, i + 1);
-                        if (substr2 == NULL) {
-                            continue;
-                        }
-                        index = strcspn(propertyValue+pos, substr2);
-                        if (index == strlen(propertyValue+pos)) {
-                            *out = false;
-                            return CELIX_SUCCESS;
-                        }
-
-                        pos = index + strlen(substr2);
-                        if (i + 2 < size) {
-                            i++;
-                        }
-                    } else {
-                        unsigned int len = strlen(substr);
-                        char * region = (char *)calloc(1, len+1);
-                        strncpy(region, propertyValue+pos, len);
-                        region[len]    = '\0';
-                        if (strcmp(region, substr) == 0) {
-                            pos += len;
-                        } else {
-                            free(region);
-                            *out = false;
-                            return CELIX_SUCCESS;
-                        }
-                        free(region);
-                    }
-                } else {
-                    unsigned int len;
-                    int begin;
+    const char* currentValue = entry->value;
 
-                    if (substr == NULL) {
-                        *out = true;
-                        return CELIX_SUCCESS;
-                    }
-                    len = strlen(substr);
-                    begin = strlen(propertyValue)-len;
-                    *out = (strcmp(propertyValue+begin, substr) == 0);
-                    return CELIX_SUCCESS;
-                }
-            }
-            *out = true;
-            return CELIX_SUCCESS;
-        }
-        case CELIX_FILTER_OPERAND_APPROX: {
-            *out = strcasecmp(propertyValue, filter->value) == 0;
-            return CELIX_SUCCESS;
-        }
-        case CELIX_FILTER_OPERAND_EQUAL: {
-            *out = (celix_filter_compareAttributeValue(filter, propertyValue) 
== 0);
-            return CELIX_SUCCESS;
-        }
-        case CELIX_FILTER_OPERAND_GREATER: {
-            *out = (celix_filter_compareAttributeValue(filter, propertyValue) 
> 0);
-            return CELIX_SUCCESS;
-        }
-        case CELIX_FILTER_OPERAND_GREATEREQUAL: {
-            *out = (celix_filter_compareAttributeValue(filter, propertyValue) 
>= 0);
-            return CELIX_SUCCESS;
-        }
-        case CELIX_FILTER_OPERAND_LESS: {
-            *out = (celix_filter_compareAttributeValue(filter, propertyValue) 
< 0);
-            return CELIX_SUCCESS;
+    if (initial) {
+        const char* found = strstr(entry->value, initial);
+        currentValue = found + celix_utils_strlen(initial);
+        if (!found || found != entry->value) {
+            return false;
         }
-        case CELIX_FILTER_OPERAND_LESSEQUAL: {
-            *out = (celix_filter_compareAttributeValue(filter, propertyValue) 
<= 0);
-            return CELIX_SUCCESS;
+    }
+
+    for (int i = 1; i < celix_arrayList_size(filter->children) - 1; i++) {
+        const char* substr = celix_arrayList_get(filter->children, i);
+        const char* found = strstr(currentValue, substr);
+        if (!found || found <= currentValue) {
+            return false;
         }
-        case CELIX_FILTER_OPERAND_AND:
-        case CELIX_FILTER_OPERAND_NOT:
-        case CELIX_FILTER_OPERAND_OR:
-        case CELIX_FILTER_OPERAND_PRESENT: {
+        currentValue = found + celix_utils_strlen(substr);
+    }
+
+    if (final) {
+        const char* found = strstr(currentValue, final);
+        if (!found || found <= currentValue || found + 
celix_utils_strlen(final) != entry->value + strLen) {
+            return false;
         }
-            /* no break */
     }
 
-    if (out != NULL) {
-        *out = result;
+    return true;
+}
+
+static bool celix_filter_matchPropertyEntry(const celix_filter_t* filter, 
const celix_properties_entry_t* entry) {
+    switch (filter->operand) {
+    case CELIX_FILTER_OPERAND_SUBSTRING:
+        return celix_filter_matchSubString(filter, entry);
+    case CELIX_FILTER_OPERAND_APPROX:
+        return strcasecmp(entry->value, filter->value) == 0;
+    case CELIX_FILTER_OPERAND_EQUAL:
+        return celix_filter_compareAttributeValue(filter, entry) == 0;
+    case CELIX_FILTER_OPERAND_GREATER:
+        return celix_filter_compareAttributeValue(filter, entry) > 0;
+    case CELIX_FILTER_OPERAND_GREATEREQUAL:
+        return celix_filter_compareAttributeValue(filter, entry) >= 0;
+    case CELIX_FILTER_OPERAND_LESS:
+        return celix_filter_compareAttributeValue(filter, entry) < 0;
+    default:
+        assert(filter->operand == CELIX_FILTER_OPERAND_LESSEQUAL);
+        return celix_filter_compareAttributeValue(filter, entry) <= 0;
     }
-    return status;
 }
 
-celix_status_t filter_getString(celix_filter_t * filter, const char 
**filterStr) {
+celix_status_t filter_getString(celix_filter_t* filter, const char** 
filterStr) {
     if (filter != NULL) {
         *filterStr = filter->filterStr;
     }
     return CELIX_SUCCESS;
 }
 
-celix_status_t filter_match_filter(celix_filter_t *src, celix_filter_t *dest, 
bool *out) {
-    bool result = celix_filter_matchFilter(src, dest);
+celix_status_t filter_match_filter(celix_filter_t* src, celix_filter_t* dest, 
bool* out) {
+    bool result = celix_filter_equals(src, dest);
     if (out != NULL) {
         *out = result;
     }
     return CELIX_SUCCESS;
 }
 
+celix_filter_t* celix_filter_create(const char* filterString) {
+    if (!filterString || strnlen(filterString, 1) == 0) {

Review Comment:
   ```suggestion
       if (!filterString || filterString[0] == '\0') {
   ```



##########
libs/utils/src/filter.c:
##########
@@ -17,421 +17,516 @@
  * 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 convertedToVersion;
-    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);
+    bool convertedToBool;
+    bool boolValue;
 
-static celix_status_t filter_compare(const celix_filter_t* filter, const char 
*propertyValue, bool *result);
+    bool convertedToVersion;
+    celix_version_t* versionValue;
+};
 
-static void filter_skipWhiteSpace(char * filterString, int * pos) {
-    int length;
+static void celix_filter_skipWhiteSpace(char* filterString, int* pos);
+static celix_filter_t* celix_filter_parseFilter(char* filterString, int* pos);
+static celix_filter_t* celix_filter_parseFilterNode(char* filterString, int* 
pos);
+static celix_filter_t* celix_filter_parseAndOrOr(char* filterString, 
celix_filter_operand_t andOrOr, int* pos);
+static celix_filter_t* celix_filter_parseNot(char* filterString, int* pos);
+static celix_filter_t* celix_filter_parseItem(char* filterString, int* pos);
+static char* celix_filter_parseAttributeOrValue(const char* filterString, int* 
pos, bool parseAttribute);
+static celix_array_list_t* celix_filter_parseSubstring(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(char* filterString, int* pos) {
+    size_t length;
     for (length = strlen(filterString); (*pos < length) && 
isspace(filterString[*pos]);) {
         (*pos)++;

Review Comment:
   ```suggestion
       for (; filterString[*pos] != '\0' && isspace(filterString[*pos]);) {
   ```
   
   This should eliminate lots of `strlen` calls.



##########
libs/utils/src/filter.c:
##########
@@ -17,421 +17,516 @@
  * 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 convertedToVersion;
-    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);
+    bool convertedToBool;
+    bool boolValue;
 
-static celix_status_t filter_compare(const celix_filter_t* filter, const char 
*propertyValue, bool *result);
+    bool convertedToVersion;
+    celix_version_t* versionValue;
+};
 
-static void filter_skipWhiteSpace(char * filterString, int * pos) {
-    int length;
+static void celix_filter_skipWhiteSpace(char* filterString, int* pos);
+static celix_filter_t* celix_filter_parseFilter(char* filterString, int* pos);
+static celix_filter_t* celix_filter_parseFilterNode(char* filterString, int* 
pos);
+static celix_filter_t* celix_filter_parseAndOrOr(char* filterString, 
celix_filter_operand_t andOrOr, int* pos);
+static celix_filter_t* celix_filter_parseNot(char* filterString, int* pos);
+static celix_filter_t* celix_filter_parseItem(char* filterString, int* pos);
+static char* celix_filter_parseAttributeOrValue(const char* filterString, int* 
pos, bool parseAttribute);
+static celix_array_list_t* celix_filter_parseSubstring(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(char* filterString, int* pos) {
+    size_t length;
     for (length = strlen(filterString); (*pos < length) && 
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(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);
 
-    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(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) {
+static celix_filter_t* celix_filter_parseAndOrOr(char* filterString, 
celix_filter_operand_t andOrOr, int* pos) {
+    celix_filter_skipWhiteSpace(filterString, pos);
 
-    filter_skipWhiteSpace(filterString, pos);
-    bool failure = false;
+    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;
+    }
 
-    if (filterString[*pos] != '(') {
-        fprintf(stderr, "Filter Error: Missing '('.\n");
+    celix_autoptr(celix_array_list_t) children = celix_arrayList_create();
+    if (!children) {
+        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;
+    if (filterString[*pos] == ')') {
+        // empty and/or filter
+    } else if (filterString[*pos] != '(') {
+        celix_err_push("Filter Error: Missing '('.");
+        return NULL;
+    } else {
+        bool failure = false;
+        while (filterString[*pos] == '(') {
+            celix_filter_t* child = celix_filter_parseFilter(filterString, 
pos);
+            if (child == NULL) {
+                failure = true;
+                break;
+            }
+            celix_arrayList_add(children, child);
         }
-        celix_arrayList_add(children, child);
-    }
 
-    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);
+        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);
+            }
+            return NULL;
         }
-        celix_arrayList_destroy(children);
-        return NULL;
     }
 
-    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(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;
     }
 
-    child = filter_parseFilter(filterString, pos);
-    if (child == NULL) {
+    celix_filter_t* child = celix_filter_parseFilter(filterString, pos);
+    if (!child) {
         return NULL;
     }
+
     celix_array_list_t* children = celix_arrayList_create();
+    if (!children) {
+        celix_err_push("Filter Error: Failed to allocate memory.");
+        celix_filter_destroy(child);
+        return NULL;
+    }
     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_filter_destroy(child);
+        celix_arrayList_destroy(children);
+        return NULL;
+    }
 
+    filter->operand = CELIX_FILTER_OPERAND_NOT;
+    filter->children = children; //note can be NULL
     return filter;
 }
 
-static celix_filter_t * filter_parseItem(char * filterString, int * pos) {
-    char * attr = filter_parseAttr(filterString, pos);
-    if(attr == NULL){
+static celix_filter_t* celix_filter_parseItem(char* filterString, int* pos) {
+    celix_autofree char* attr = 
celix_filter_parseAttributeOrValue(filterString, pos, true);
+    if (!attr) {
+        celix_err_push("Filter Error: Missing attr.");
         return NULL;
     }
 
-    filter_skipWhiteSpace(filterString, pos);
-    switch(filterString[*pos]) {
-        case '~': {
-            if (filterString[*pos + 1] == '=') {
-                celix_filter_t * filter = (celix_filter_t *) calloc(1, 
sizeof(*filter));
-                *pos += 2;
-                filter->operand = CELIX_FILTER_OPERAND_APPROX;
-                filter->attribute = attr;
-                filter->value = filter_parseValue(filterString, pos);
-                return filter;
+    celix_autoptr(celix_filter_t) filter = calloc(1, sizeof(*filter));
+    if (!filter) {
+        celix_err_push("Filter Error: Failed to allocate memory.");
+        return NULL;
+    }
+    filter->attribute = celix_steal_ptr(attr);
+
+    celix_filter_skipWhiteSpace(filterString, pos);
+    char op = filterString[*pos];
+    switch (op) {
+    case '~': {
+        char secondOp = filterString[*pos + 1];
+        if (secondOp == '=') {
+            *pos += 2;
+            filter->operand = CELIX_FILTER_OPERAND_APPROX;
+            filter->value = celix_filter_parseAttributeOrValue(filterString, 
pos, false);
+            if (!filter->value) {
+                return NULL;
             }
-            break;
+            return celix_steal_ptr(filter);
         }
-        case '>': {
-            if (filterString[*pos + 1] == '=') {
-                celix_filter_t * filter = (celix_filter_t *) calloc(1, 
sizeof(*filter));
-                *pos += 2;
-                filter->operand = CELIX_FILTER_OPERAND_GREATEREQUAL;
-                filter->attribute = attr;
-                filter->value = filter_parseValue(filterString, pos);
-                return filter;
+        celix_err_pushf("Filter Error: Invalid operand char after ~. Expected 
`=` got `%c`", secondOp);
+        return NULL;
+    }
+    case '>': {
+        if (filterString[*pos + 1] == '=') {
+            *pos += 2;
+            filter->operand = CELIX_FILTER_OPERAND_GREATEREQUAL;
+            filter->value = celix_filter_parseAttributeOrValue(filterString, 
pos, false);
+            if (!filter->value) {
+                return NULL;
             }
-            else {
-                celix_filter_t * filter = (celix_filter_t *) calloc(1, 
sizeof(*filter));
-                *pos += 1;
-                filter->operand = CELIX_FILTER_OPERAND_GREATER;
-                filter->attribute = attr;
-                filter->value = filter_parseValue(filterString, pos);
-                return filter;
+            return celix_steal_ptr(filter);
+        } else {
+            *pos += 1;
+            filter->operand = CELIX_FILTER_OPERAND_GREATER;
+            filter->value = celix_filter_parseAttributeOrValue(filterString, 
pos, false);
+            if (!filter->value) {
+                return NULL;
             }
-            break;
+            return celix_steal_ptr(filter);
         }
-        case '<': {
-            if (filterString[*pos + 1] == '=') {
-                celix_filter_t * filter = (celix_filter_t *) calloc(1, 
sizeof(*filter));
-                *pos += 2;
-                filter->operand = CELIX_FILTER_OPERAND_LESSEQUAL;
-                filter->attribute = attr;
-                filter->value = filter_parseValue(filterString, pos);
-                return filter;
+    }
+    case '<': {
+        if (filterString[*pos + 1] == '=') {
+            *pos += 2;
+            filter->operand = CELIX_FILTER_OPERAND_LESSEQUAL;
+            filter->value = celix_filter_parseAttributeOrValue(filterString, 
pos, false);
+            if (!filter->value) {
+                return NULL;
             }
-            else {
-                celix_filter_t * filter = (celix_filter_t *) calloc(1, 
sizeof(*filter));
-                *pos += 1;
-                filter->operand = CELIX_FILTER_OPERAND_LESS;
-                filter->attribute = attr;
-                filter->value = filter_parseValue(filterString, pos);
-                return filter;
+            return celix_steal_ptr(filter);
+        } else {
+            *pos += 1;
+            filter->operand = CELIX_FILTER_OPERAND_LESS;
+            filter->value = celix_filter_parseAttributeOrValue(filterString, 
pos, false);
+            if (!filter->value) {
+                return NULL;
             }
-            break;
-        }
-        case '=': {
-            celix_filter_t * filter = NULL;
-            celix_array_list_t *subs;
-            if (filterString[*pos + 1] == '*') {
-                int oldPos = *pos;
-                *pos += 2;
-                filter_skipWhiteSpace(filterString, pos);
-                if (filterString[*pos] == ')') {
-                    filter = (celix_filter_t *) calloc(1, sizeof(*filter));
-                    filter->operand = CELIX_FILTER_OPERAND_PRESENT;
-                    filter->attribute = attr;
-                    filter->value = NULL;
-                    return filter;
-                }
-                *pos = oldPos;
+            return celix_steal_ptr(filter);
+        }
+    }
+    case '=': {
+        if (filterString[*pos + 1] == '*') {
+            int oldPos = *pos;
+            *pos += 2;
+            celix_filter_skipWhiteSpace(filterString, pos);
+            if (filterString[*pos] == ')') {
+                filter->operand = CELIX_FILTER_OPERAND_PRESENT;
+                filter->value = NULL;
+                return celix_steal_ptr(filter);
             }
-            filter = (celix_filter_t *) calloc(1, sizeof(*filter));            
-            (*pos)++;
-            subs = filter_parseSubstring(filterString, pos);
-            if(subs!=NULL){
-                if (celix_arrayList_size(subs) == 1) {
-                    char * string = (char *) celix_arrayList_get(subs, 0);
-                    if (string != NULL) {
-                        filter->operand = CELIX_FILTER_OPERAND_EQUAL;
-                        filter->attribute = attr;
-                        filter->value = string;
-
-                        celix_arrayList_clear(subs);
-                        celix_arrayList_destroy(subs);
-
-                        return filter;
-                    }
-                }
+            *pos = oldPos;
+        }
+        (*pos)++;
+        if (celix_filter_isSubString(filterString, *pos)) {
+            celix_array_list_t* subs = 
celix_filter_parseSubstring(filterString, pos);
+            if (!subs) {
+                return NULL;
             }
             filter->operand = CELIX_FILTER_OPERAND_SUBSTRING;
-            filter->attribute = attr;
             filter->children = subs;
-            return filter;
-        }
-    }
-    fprintf(stderr, "Filter Error: Invalid operator.\n");
-    free(attr);
-    return NULL;
-}
-
-static char * filter_parseAttr(char * filterString, int * pos) {
-    char c;
-    int begin = *pos;
-    int end = *pos;
-    int length = 0;
-
-    filter_skipWhiteSpace(filterString, pos);
-    c = filterString[*pos];
-
-    while (c != '~' && c != '<' && c != '>' && c != '=' && c != '(' && c != 
')') {
-        (*pos)++;
-
-        if (!isspace(c)) {
-            end = *pos;
+            return celix_steal_ptr(filter);
+        } else {
+            filter->operand = CELIX_FILTER_OPERAND_EQUAL;
+            filter->value = celix_filter_parseAttributeOrValue(filterString, 
pos, false);
+            if (!filter->value) {
+                return NULL;
+            }
+            return celix_steal_ptr(filter);
         }
-
-        c = filterString[*pos];
     }
-
-    length = end - begin;
-
-    if (length == 0) {
-        fprintf(stderr, "Filter Error: Missing attr.\n");
+    default: {
+        celix_err_pushf("Filter Error: Invalid operand char `%c`", op);
         return NULL;
-    } else {
-        char * attr = (char *) calloc(1, length+1);
-        strncpy(attr, filterString+begin, length);
-        attr[length] = '\0';
-        return attr;
+    }
     }
 }
 
-static char * filter_parseValue(char * filterString, int * pos) {
-    char *value = calloc(strlen(filterString) + 1, sizeof(*value));
-    int keepRunning = 1;
+static char* celix_filter_parseAttributeOrValue(const char* filterString, int* 
pos, bool parseAttribute) {
+    const char* name = parseAttribute ? "attribute" : "attribute value";
+    celix_autofree char* value = NULL;
+    size_t valueSize = 0;
+    celix_autoptr(FILE) stream = open_memstream(&value, &valueSize);
+    if (!stream) {
+        celix_err_push("Filter Error: Failed to open mem stream.");
+        return NULL;
+    }
 
+    bool keepRunning = true;
     while (keepRunning) {
         char c = filterString[*pos];
 
         switch (c) {
-            case ')': {
-                keepRunning = 0;
-                break;
-            }
-            case '(': {
-                fprintf(stderr, "Filter Error: Invalid value.\n");
-                free(value);
+        case ')': {
+            if (parseAttribute) {
+                celix_err_pushf("Filter Error: Unexpected `)` char while 
parsing %s.", name);
+                fflush(stream);

Review Comment:
   `fflush` could be removed, since `fclose` that would be called automatically 
should be enough. This applies to all the following usages of `fflush`.



##########
libs/utils/src/filter.c:
##########
@@ -17,421 +17,516 @@
  * 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 convertedToVersion;
-    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);
+    bool convertedToBool;
+    bool boolValue;
 
-static celix_status_t filter_compare(const celix_filter_t* filter, const char 
*propertyValue, bool *result);
+    bool convertedToVersion;
+    celix_version_t* versionValue;
+};
 
-static void filter_skipWhiteSpace(char * filterString, int * pos) {
-    int length;
+static void celix_filter_skipWhiteSpace(char* filterString, int* pos);
+static celix_filter_t* celix_filter_parseFilter(char* filterString, int* pos);
+static celix_filter_t* celix_filter_parseFilterNode(char* filterString, int* 
pos);
+static celix_filter_t* celix_filter_parseAndOrOr(char* filterString, 
celix_filter_operand_t andOrOr, int* pos);
+static celix_filter_t* celix_filter_parseNot(char* filterString, int* pos);
+static celix_filter_t* celix_filter_parseItem(char* filterString, int* pos);
+static char* celix_filter_parseAttributeOrValue(const char* filterString, int* 
pos, bool parseAttribute);
+static celix_array_list_t* celix_filter_parseSubstring(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(char* filterString, int* pos) {
+    size_t length;
     for (length = strlen(filterString); (*pos < length) && 
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(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);
 
-    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(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) {
+static celix_filter_t* celix_filter_parseAndOrOr(char* filterString, 
celix_filter_operand_t andOrOr, int* pos) {
+    celix_filter_skipWhiteSpace(filterString, pos);
 
-    filter_skipWhiteSpace(filterString, pos);
-    bool failure = false;
+    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;
+    }
 
-    if (filterString[*pos] != '(') {
-        fprintf(stderr, "Filter Error: Missing '('.\n");
+    celix_autoptr(celix_array_list_t) children = celix_arrayList_create();
+    if (!children) {
+        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;
+    if (filterString[*pos] == ')') {
+        // empty and/or filter
+    } else if (filterString[*pos] != '(') {
+        celix_err_push("Filter Error: Missing '('.");
+        return NULL;
+    } else {
+        bool failure = false;
+        while (filterString[*pos] == '(') {
+            celix_filter_t* child = celix_filter_parseFilter(filterString, 
pos);
+            if (child == NULL) {
+                failure = true;
+                break;
+            }
+            celix_arrayList_add(children, child);
         }
-        celix_arrayList_add(children, child);
-    }
 
-    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);
+        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);
+            }
+            return NULL;
         }
-        celix_arrayList_destroy(children);
-        return NULL;
     }
 
-    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(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;
     }
 
-    child = filter_parseFilter(filterString, pos);
-    if (child == NULL) {
+    celix_filter_t* child = celix_filter_parseFilter(filterString, pos);
+    if (!child) {
         return NULL;
     }
+
     celix_array_list_t* children = celix_arrayList_create();
+    if (!children) {
+        celix_err_push("Filter Error: Failed to allocate memory.");
+        celix_filter_destroy(child);
+        return NULL;
+    }
     celix_arrayList_add(children, child);

Review Comment:
   Error handling is needed.



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