PengZheng commented on code in PR #721:
URL: https://github.com/apache/celix/pull/721#discussion_r1474393676
##########
libs/utils/include/celix_properties.h:
##########
@@ -385,14 +512,547 @@ celix_properties_getVersion(const celix_properties_t*
properties, const char* ke
* @param[in] properties The property set to search.
* @param[in] key The key of the property to get.
* @param[in] defaultValue The value to return if the property is not set or
if the value is not a Celix version.
- * @return A copy of the property value if it is a Celix version, or a new
Celix version created from the property
- * value string if it can be converted, or a copy of the default value
if the property is not set or the value
- * is not a valid Celix version.
- * @retval NULL if version cannot be found/converted and the defaultValue is
NULL.
+ * @param[out] version A copy of the found version, a new parsed version, or a
copy of the default value if the
+ * property is not set, its value is not an version or its
value cannot be converted to an version.
+ * @return CELIX_SUCCESS if the operation was successful, CELIX_ENOMEM if
there was not enough memory to create the
+ * version. Note if the key is not found, the return status is still
CELIX_SUCCESS.
*/
-CELIX_UTILS_EXPORT celix_version_t* celix_properties_getAsVersion(const
celix_properties_t* properties,
+CELIX_UTILS_EXPORT celix_status_t celix_properties_getAsVersion(const
celix_properties_t* properties,
const char*
key,
- const
celix_version_t* defaultValue);
+ const
celix_version_t* defaultValue,
+
celix_version_t** version);
+
+/**
+ * @brief Set a long array value for a property.
+ *
+ * The set property type will be CELIX_PROPERTIES_VALUE_TYPE_LONG_ARRAY.
+ *
+ * This function will make a copy of the provided celix_array_list_t object,
assuming it contains long values,
+ * and store it in the property set.
+ * If an error occurs, the error status is returned and a message is logged to
+ * celix_err.
+ *
+ * @param[in] properties The property set to modify.
+ * @param[in] key The key of the property to set.
+ * @param[in] values An array list of long values to set for the property.
Cannot be NULL.
+ * @return CELIX_SUCCESS if the operation was successful, CELIX_ENOMEM if
there was not enough memory to set the entry,
+ * and CELIX_ILLEGAL_ARGUMENT if the provided key or values is NULL.
+ */
+CELIX_UTILS_EXPORT celix_status_t
celix_properties_setLongArrayList(celix_properties_t* properties,
Review Comment:
I feel this part adds lots of duplicated code. If we only add
setArrayList/assignArrayList/getArrayList, then the concrete type of element in
array is left to arrayList. The caller can first get an arrayList then decide
the element type. This way, large part of duplication will be eliminated.
##########
libs/utils/include/celix_properties.h:
##########
@@ -238,10 +256,83 @@ CELIX_UTILS_EXPORT celix_status_t
celix_properties_set(celix_properties_t* prope
* and CELIX_ILLEGAL_ARGUMENT if the provided key or value is NULL.
* When an error status is returned, the key and value will be freed
by this function.
*/
-CELIX_UTILS_EXPORT celix_status_t
celix_properties_setWithoutCopy(celix_properties_t* properties,
+CELIX_UTILS_EXPORT celix_status_t celix_properties_assign(celix_properties_t*
properties,
char* key,
Review Comment:
This seems quite unnatural and different from other assign functions. `const
char*` will be better. The only usage in current code base, i.e.
`rsaShm_createEndpointDescription`, can be replaced by
`celix_properties_assignString`.
##########
libs/utils/include/celix_version.h:
##########
@@ -91,11 +98,39 @@ CELIX_UTILS_EXPORT celix_version_t*
celix_version_copy(const celix_version_t* ve
*
* There must be no whitespace in version.
*
+ * If the return is NULL, an error message is logged to celix_err.
+ *
* @param[in] versionStr String representation of the version identifier.
- * @return The created version or NULL if the input was invalid.
+ * @return The created version or NULL if the input was invalid or memory
could not be allocated.
*/
CELIX_UTILS_EXPORT celix_version_t*
celix_version_createVersionFromString(const char *versionStr);
+//TODO test
Review Comment:
Should this be removed now?
##########
libs/utils/include/celix_filter_type.h:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * @file celix_filter_type.h
Review Comment:
I just realized this is an embodiment of #715.
##########
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
+ return CELIX_SUCCESS;
+ }
+ return convertStatus;
+ }
+ if (defaultValue) {
+ *list = celix_arrayList_copy(defaultValue);
+ return *list ? CELIX_SUCCESS : CELIX_ENOMEM;
+ }
+ *list = NULL;
+ return CELIX_SUCCESS;
+}
+
+celix_status_t
+celix_properties_setLongArrayList(celix_properties_t* properties, const char*
key, const celix_array_list_t* values) {
+ assert(values != NULL);
Review Comment:
As `celix_utils_convertStringToXXXArrayList`, all these `setXXXArrayList`
suffer from lack of type checking. What if `val` is not a `LongArrayList`?
--
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
