PengZheng commented on code in PR #721:
URL: https://github.com/apache/celix/pull/721#discussion_r1464647440
##########
libs/utils/src/properties.c:
##########
@@ -809,11 +910,518 @@ celix_properties_setVersion(celix_properties_t* props,
const char* key, const ce
return celix_properties_createAndSetEntry(props, key, &prototype);
}
-celix_status_t celix_properties_setVersionWithoutCopy(celix_properties_t*
props, const char* key, celix_version_t* version) {
+celix_status_t
+celix_properties_assignVersion(celix_properties_t* properties, const char*
key, celix_version_t* version) {
+ assert(version != NULL);
celix_properties_entry_t prototype = {0};
prototype.valueType = CELIX_PROPERTIES_VALUE_TYPE_VERSION;
prototype.typed.versionValue = version;
- return celix_properties_createAndSetEntry(props, key, &prototype);
+ return celix_properties_createAndSetEntry(properties, key, &prototype);
+}
+
+celix_status_t celix_properties_getAsLongArrayList(const celix_properties_t*
properties,
+ const char* key,
+ const celix_array_list_t*
defaultValue,
+ celix_array_list_t** list) {
+ const celix_properties_entry_t* entry =
celix_properties_getEntry(properties, key);
+ if (entry != NULL && entry->valueType ==
CELIX_PROPERTIES_VALUE_TYPE_LONG_ARRAY) {
+ celix_array_list_t* copy =
celix_arrayList_copy(entry->typed.arrayValue);
+ if (!copy) {
+ return CELIX_ENOMEM;
+ }
+ *list = copy;
+ return CELIX_SUCCESS;
+ }
+ if (entry != NULL && entry->valueType ==
CELIX_PROPERTIES_VALUE_TYPE_STRING) {
+ celix_status_t convertStatus =
celix_utils_convertStringToLongArrayList(entry->value, defaultValue, list);
+ if (convertStatus == CELIX_ILLEGAL_ARGUMENT) {
+ // conversion failed, but no memory error so defaultValue is
copied and set
Review Comment:
The natural semantics of default value is that if the key is absent, use the
provided value instead. But here the semantics is that if the type is
incorrect, use the provided value instead. It feels somewhat unnatural.
##########
libs/utils/src/celix_convert_utils.c:
##########
@@ -107,25 +110,242 @@ long celix_utils_convertStringToLong(const char* val,
long defaultValue, bool* c
return result;
}
-celix_version_t* celix_utils_convertStringToVersion(const char* val, const
celix_version_t* defaultValue, bool* converted) {
- celix_version_t* result = NULL;
- if (val != NULL) {
- //check if string has two dots ('.'), and only try to create string if
it has two dots
- char* firstDot = strchr(val, '.');
- char* lastDot = strrchr(val, '.');
- if (firstDot != NULL && lastDot != NULL && firstDot != lastDot) {
- char buf[64];
- char* valCopy = celix_utils_writeOrCreateString(buf, sizeof(buf),
"%s", val);
- char *trimmed = celix_utils_trimInPlace(valCopy);
- result = celix_version_createVersionFromString(trimmed);
- celix_utils_freeStringIfNotEqual(buf, valCopy);
+celix_status_t
+celix_utils_convertStringToVersion(const char* val, const celix_version_t*
defaultValue, celix_version_t** version) {
+ assert(version != NULL);
+ if (!val && defaultValue) {
+ *version = celix_version_copy(defaultValue);
+ return *version ? CELIX_ILLEGAL_ARGUMENT : CELIX_ENOMEM;
+ } else if (!val) {
+ return CELIX_ILLEGAL_ARGUMENT;
+ }
+
+ celix_status_t status = celix_version_parse(val, version);
+ if (status == CELIX_ILLEGAL_ARGUMENT) {
+ if (defaultValue) {
+ *version = celix_version_copy(defaultValue);
+ return *version ? status : CELIX_ENOMEM;
}
+ return status;
+ }
+ return status;
+}
+
+/**
+ * @brief Convert the provided string to an array list using the provided
addEntry callback to convert the string
+ * to a specific type and add it to the list.
+ */
+static celix_status_t celix_utils_convertStringToArrayList(const char* val,
+ const
celix_array_list_t* defaultValue,
+
celix_array_list_t** list,
+ void
(*freeCb)(void*),
+ celix_status_t
(*addEntry)(celix_array_list_t*,
+
const char*)) {
+ assert(list != NULL);
+ *list = NULL;
+
+ if (!val && defaultValue) {
+ *list = celix_arrayList_copy(defaultValue);
Review Comment:
Seeing this usage, I feel strongly that shallow copy is inconsistent and
error-prone. Imagine that after `celix_utils_convertStringToArrayList` returns,
`defaultValue` is destroyed and the result is hold for later use, which will
lead to use-after-free.
##########
libs/utils/src/properties.c:
##########
@@ -809,11 +910,518 @@ celix_properties_setVersion(celix_properties_t* props,
const char* key, const ce
return celix_properties_createAndSetEntry(props, key, &prototype);
}
-celix_status_t celix_properties_setVersionWithoutCopy(celix_properties_t*
props, const char* key, celix_version_t* version) {
+celix_status_t
+celix_properties_assignVersion(celix_properties_t* properties, const char*
key, celix_version_t* version) {
+ assert(version != NULL);
celix_properties_entry_t prototype = {0};
prototype.valueType = CELIX_PROPERTIES_VALUE_TYPE_VERSION;
prototype.typed.versionValue = version;
- return celix_properties_createAndSetEntry(props, key, &prototype);
+ return celix_properties_createAndSetEntry(properties, key, &prototype);
+}
+
+celix_status_t celix_properties_getAsLongArrayList(const celix_properties_t*
properties,
+ const char* key,
+ const celix_array_list_t*
defaultValue,
+ celix_array_list_t** list) {
+ const celix_properties_entry_t* entry =
celix_properties_getEntry(properties, key);
+ if (entry != NULL && entry->valueType ==
CELIX_PROPERTIES_VALUE_TYPE_LONG_ARRAY) {
+ celix_array_list_t* copy =
celix_arrayList_copy(entry->typed.arrayValue);
+ if (!copy) {
+ return CELIX_ENOMEM;
+ }
+ *list = copy;
+ return CELIX_SUCCESS;
+ }
+ if (entry != NULL && entry->valueType ==
CELIX_PROPERTIES_VALUE_TYPE_STRING) {
+ celix_status_t convertStatus =
celix_utils_convertStringToLongArrayList(entry->value, defaultValue, list);
+ if (convertStatus == CELIX_ILLEGAL_ARGUMENT) {
+ // conversion failed, but no memory error so defaultValue is
copied and set
Review Comment:
The user may need to know these kinds of type mistches.
##########
libs/utils/src/celix_convert_utils.c:
##########
@@ -107,25 +110,242 @@ long celix_utils_convertStringToLong(const char* val,
long defaultValue, bool* c
return result;
}
-celix_version_t* celix_utils_convertStringToVersion(const char* val, const
celix_version_t* defaultValue, bool* converted) {
- celix_version_t* result = NULL;
- if (val != NULL) {
- //check if string has two dots ('.'), and only try to create string if
it has two dots
- char* firstDot = strchr(val, '.');
- char* lastDot = strrchr(val, '.');
- if (firstDot != NULL && lastDot != NULL && firstDot != lastDot) {
- char buf[64];
- char* valCopy = celix_utils_writeOrCreateString(buf, sizeof(buf),
"%s", val);
- char *trimmed = celix_utils_trimInPlace(valCopy);
- result = celix_version_createVersionFromString(trimmed);
- celix_utils_freeStringIfNotEqual(buf, valCopy);
+celix_status_t
+celix_utils_convertStringToVersion(const char* val, const celix_version_t*
defaultValue, celix_version_t** version) {
+ assert(version != NULL);
+ if (!val && defaultValue) {
+ *version = celix_version_copy(defaultValue);
+ return *version ? CELIX_ILLEGAL_ARGUMENT : CELIX_ENOMEM;
+ } else if (!val) {
+ return CELIX_ILLEGAL_ARGUMENT;
+ }
+
+ celix_status_t status = celix_version_parse(val, version);
+ if (status == CELIX_ILLEGAL_ARGUMENT) {
+ if (defaultValue) {
+ *version = celix_version_copy(defaultValue);
+ return *version ? status : CELIX_ENOMEM;
}
+ return status;
+ }
+ return status;
+}
+
+/**
+ * @brief Convert the provided string to an array list using the provided
addEntry callback to convert the string
+ * to a specific type and add it to the list.
+ */
+static celix_status_t celix_utils_convertStringToArrayList(const char* val,
+ const
celix_array_list_t* defaultValue,
+
celix_array_list_t** list,
+ void
(*freeCb)(void*),
+ celix_status_t
(*addEntry)(celix_array_list_t*,
+
const char*)) {
+ assert(list != NULL);
+ *list = NULL;
+
+ if (!val && defaultValue) {
+ *list = celix_arrayList_copy(defaultValue);
+ return *list ? CELIX_ILLEGAL_ARGUMENT : CELIX_ENOMEM;
+ } else if (!val) {
+ return CELIX_ILLEGAL_ARGUMENT;
+ }
+
+ celix_array_list_create_options_t opts =
CELIX_EMPTY_ARRAY_LIST_CREATE_OPTIONS;
+ opts.simpleRemovedCallback = freeCb;
+ celix_autoptr(celix_array_list_t) result =
celix_arrayList_createWithOptions(&opts);
+ if (!result) {
+ return CELIX_ENOMEM;
+ }
+
+ char buf[256];
+ char* valCopy = celix_utils_writeOrCreateString(buf, sizeof(buf), "%s",
val);
+ if (!valCopy) {
+ return CELIX_ENOMEM;
}
- if (converted) {
- *converted = result != NULL;
+
+ celix_status_t status = CELIX_SUCCESS;
+ char* savePtr = NULL;
+ char* token = strtok_r(valCopy, ",", &savePtr);
+ while (token != NULL) {
+ status = addEntry(result, token);
+ if (status != CELIX_SUCCESS) {
+ break;
+ }
+ token = strtok_r(NULL, ",", &savePtr);
}
- if (result == NULL && defaultValue != NULL) {
- result = celix_version_copy(defaultValue);
+ celix_utils_freeStringIfNotEqual(buf, valCopy);
+
+ if (status == CELIX_SUCCESS) {
+ *list = celix_steal_ptr(result);
+ } else if (status == CELIX_ILLEGAL_ARGUMENT) {
+ if (defaultValue) {
+ *list = celix_arrayList_copy(defaultValue);
+ return *list ? status : CELIX_ENOMEM;
+ }
+ return status;
}
+ return status;
+}
+
+celix_status_t celix_utils_addLongEntry(celix_array_list_t* list, const char*
entry) {
+ bool converted;
+ long l = celix_utils_convertStringToLong(entry, 0L, &converted);
+ if (!converted) {
+ return CELIX_ILLEGAL_ARGUMENT;
+ }
+ return celix_arrayList_addLong(list, l);
+}
+
+celix_status_t celix_utils_convertStringToLongArrayList(const char* val,
+ const
celix_array_list_t* defaultValue,
+ celix_array_list_t**
list) {
+ return celix_utils_convertStringToArrayList(val, defaultValue, list, NULL,
celix_utils_addLongEntry);
+}
+
+celix_status_t celix_utils_addDoubleEntry(celix_array_list_t* list, const
char* entry) {
+ bool converted;
+ double d = celix_utils_convertStringToDouble(entry, 0.0, &converted);
+ if (!converted) {
+ return CELIX_ILLEGAL_ARGUMENT;
+ }
+ return celix_arrayList_addDouble(list, d);
+}
+
+celix_status_t celix_utils_convertStringToDoubleArrayList(const char* val,
+ const
celix_array_list_t* defaultValue,
+ celix_array_list_t**
list) {
+ return celix_utils_convertStringToArrayList(val, defaultValue, list, NULL,
celix_utils_addDoubleEntry);
+}
+
+celix_status_t celix_utils_addBoolEntry(celix_array_list_t* list, const char*
entry) {
+ bool converted;
+ bool b = celix_utils_convertStringToBool(entry, 0.0, &converted);
+ if (!converted) {
+ return CELIX_ILLEGAL_ARGUMENT;
+ }
+ return celix_arrayList_addBool(list, b);
+}
+
+celix_status_t celix_utils_convertStringToBoolArrayList(const char* val,
+ const
celix_array_list_t* defaultValue,
+ celix_array_list_t**
list) {
+ return celix_utils_convertStringToArrayList(val, defaultValue, list, NULL,
celix_utils_addBoolEntry);
+}
+
+celix_status_t celix_utils_addStringEntry(celix_array_list_t* list, const
char* entry) {
+ char* copy = celix_utils_strdup(entry);
+ if (!copy) {
+ return CELIX_ENOMEM;
+ }
+ return celix_arrayList_add(list, copy);
+}
+
+celix_status_t celix_utils_convertStringToStringArrayList(const char* val,
+ const
celix_array_list_t* defaultValue,
+ celix_array_list_t**
list) {
+ return celix_utils_convertStringToArrayList(val, defaultValue, list, free,
celix_utils_addStringEntry);
+}
+
+static celix_status_t celix_utils_addVersionEntry(celix_array_list_t* list,
const char* entry) {
+ celix_version_t* version;
+ celix_status_t convertStatus = celix_utils_convertStringToVersion(entry,
NULL, &version);
+ if (convertStatus == CELIX_SUCCESS) {
+ return celix_arrayList_add(list, version);
+ }
+ return convertStatus;
+}
+
+static void celix_utils_destroyVersionEntry(void* entry) {
celix_version_destroy(entry); }
+
+celix_status_t celix_utils_convertStringToVersionArrayList(const char* val,
+ const
celix_array_list_t* defaultValue,
+
celix_array_list_t** list) {
+ return celix_utils_convertStringToArrayList(
+ val, defaultValue, list, celix_utils_destroyVersionEntry,
celix_utils_addVersionEntry);
+}
+
+/**
+ * @brief Convert the provided array list to a string using the provided print
callback.
+ *
+ * Will log an error message to celix_err if an error occurred.
+ *
+ * @param list The list to convert.
+ * @param printCb The callback to use for printing the list entries.
+ * @return The string representation of the list or NULL if an error occurred.
+ */
+static char* celix_utils_arrayListToString(const celix_array_list_t* list,
+ int (*printCb)(FILE* stream, const
celix_array_list_entry_t* entry)) {
+ char* result = NULL;
+ size_t len;
+ FILE* stream = open_memstream(&result, &len);
+ if (!stream) {
+ celix_err_push("Cannot open memstream");
+ return NULL;
+ }
+
+ int size = list ? celix_arrayList_size(list) : 0;
+ for (int i = 0; i < size; ++i) {
+ celix_array_list_entry_t entry = celix_arrayList_getEntry(list, i);
+ int rc = printCb(stream, &entry);
+ if (rc >= 0 && i < size - 1) {
+ rc = fputs(", ", stream);
+ }
+ if (rc < 0) {
+ celix_err_push("Cannot print to stream");
+ fclose(stream);
+ free(result);
+ return NULL;
+ }
+ }
+ fclose(stream);
return result;
}
+
+static int celix_utils_printLongEntry(FILE* stream, const
celix_array_list_entry_t* entry) {
+ return fprintf(stream, "%li", entry->longVal);
+}
+
+char* celix_utils_longArrayListToString(const celix_array_list_t* list) {
+ return celix_utils_arrayListToString(list, celix_utils_printLongEntry);
+}
+
+static int celix_utils_printDoubleEntry(FILE* stream, const
celix_array_list_entry_t* entry) {
+ return fprintf(stream, "%lf", entry->doubleVal);
+}
+
+char* celix_utils_doubleArrayListToString(const celix_array_list_t* list) {
+ return celix_utils_arrayListToString(list, celix_utils_printDoubleEntry);
+}
+
+static int celix_utils_printBoolEntry(FILE* stream, const
celix_array_list_entry_t* entry) {
+ return fprintf(stream, "%s", entry->boolVal ? "true" : "false");
+}
+
+char* celix_utils_boolArrayListToString(const celix_array_list_t* list) {
+ return celix_utils_arrayListToString(list, celix_utils_printBoolEntry);
+}
+
+static int celix_utils_printStrEntry(FILE* stream, const
celix_array_list_entry_t* entry) {
+ return fprintf(stream, "%s", (const char*)entry->voidPtrVal);
Review Comment:
`strVal` rather than `voidPtrVal`?
##########
libs/utils/include/celix_array_list.h:
##########
@@ -157,10 +172,27 @@ CELIX_DEFINE_AUTOPTR_CLEANUP_FUNC(celix_array_list_t,
celix_arrayList_destroy)
CELIX_UTILS_EXPORT
int celix_arrayList_size(const celix_array_list_t *list);
+/**
+ * @brief Create a shallow copy of the array list.
+ *
+ * The returned array list will be a shallow copy of the provided array list.
+ * If the entries are pointers, the pointers will be copied, but the pointed
to values will not be copied.
+ * The equals callback provided when the provided array list was created will
be copied, the removed callback
+ * will not be copied.
Review Comment:
Would reference counting be a better choice? By "better", I mean there is no
danger of dangling pointer.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
