PengZheng commented on code in PR #470:
URL: https://github.com/apache/celix/pull/470#discussion_r1380068055
##########
libs/utils/include/celix/Properties.h:
##########
@@ -145,20 +172,19 @@ namespace celix {
};
- Properties() : cProps{celix_properties_create(),
[](celix_properties_t* p) { celix_properties_destroy(p); }} {}
+ Properties() : cProps{createCProps(celix_properties_create())} {}
Properties(Properties&&) = default;
Review Comment:
After move, the stored pointer in `cProps` is nullptr? That will break class
invariant and lead to crash.
##########
libs/utils/src/properties.c:
##########
@@ -103,12 +132,275 @@ static void updateBuffers(char **key, char ** value,
char **output, int outputPo
}
}
-static void parseLine(const char* line, celix_properties_t *props) {
- int linePos = 0;
+/**
+ * Create a new string from the provided str by either using strup or storing
the string the short properties
+ * optimization string buffer.
+ */
+char* celix_properties_createString(celix_properties_t* properties, const
char* str) {
+ if (str == NULL) {
+ return (char*)CELIX_PROPERTIES_EMPTY_STRVAL;
+ }
+ size_t len = strnlen(str, CELIX_UTILS_MAX_STRLEN) + 1;
+ size_t left = CELIX_SHORT_PROPERTIES_OPTIMIZATION_STRING_BUFFER_SIZE -
properties->currentStringBufferIndex;
+ char* result;
+ if (len < left) {
+
memcpy(&properties->stringBuffer[properties->currentStringBufferIndex], str,
len);
+ result =
&properties->stringBuffer[properties->currentStringBufferIndex];
+ properties->currentStringBufferIndex += (int)len;
+ } else {
+ result = celix_utils_strdup(str);
+ }
+ return result;
+}
+/**
+ * Free string, but first check if it a static const char* const string or
part of the short properties
+ * optimization.
+ */
+static void celix_properties_freeString(celix_properties_t* properties, char*
str) {
+ if (str == CELIX_PROPERTIES_BOOL_TRUE_STRVAL || str ==
CELIX_PROPERTIES_BOOL_FALSE_STRVAL ||
+ str == CELIX_PROPERTIES_EMPTY_STRVAL) {
+ // str is static const char* const -> nop
+ } else if (str >= properties->stringBuffer &&
+ str < (properties->stringBuffer +
CELIX_SHORT_PROPERTIES_OPTIMIZATION_STRING_BUFFER_SIZE)) {
+ // str is part of the properties string buffer -> nop
+ } else {
+ free(str);
+ }
+}
+
+/**
+ * Fill entry and optional use the short properties optimization string buffer.
+ */
+static celix_status_t celix_properties_fillEntry(celix_properties_t*
properties,
+ celix_properties_entry_t*
entry,
+ const char** strValue,
+ const long* longValue,
+ const double* doubleValue,
+ const bool* boolValue,
+ celix_version_t*
versionValue) {
+ char convertedValueBuffer[32];
+ if (strValue != NULL) {
+ entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_STRING;
+ entry->value = celix_properties_createString(properties, *strValue);
+ entry->typed.strValue = entry->value;
+ } else if (longValue != NULL) {
+ entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_LONG;
+ entry->typed.longValue = *longValue;
+ int written = snprintf(convertedValueBuffer,
sizeof(convertedValueBuffer), "%li", entry->typed.longValue);
+ if (written < 0 || written >= sizeof(convertedValueBuffer)) {
+ entry->value = celix_properties_createString(properties,
convertedValueBuffer);
+ } else {
+ char* val = NULL;
+ asprintf(&val, "%li", entry->typed.longValue);
+ entry->value = val;
+ }
+ } else if (doubleValue != NULL) {
+ entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_DOUBLE;
+ entry->typed.doubleValue = *doubleValue;
+ int written = snprintf(convertedValueBuffer,
sizeof(convertedValueBuffer), "%f", entry->typed.doubleValue);
+ if (written < 0 || written >= sizeof(convertedValueBuffer)) {
+ entry->value = celix_properties_createString(properties,
convertedValueBuffer);
+ } else {
+ char* val = NULL;
+ asprintf(&val, "%f", entry->typed.doubleValue);
+ entry->value = val;
+ }
+ } else if (boolValue != NULL) {
+ entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_BOOL;
+ entry->typed.boolValue = *boolValue;
+ entry->value = entry->typed.boolValue ?
CELIX_PROPERTIES_BOOL_TRUE_STRVAL : CELIX_PROPERTIES_BOOL_FALSE_STRVAL;
+ } else /*versionValue*/ {
+ assert(versionValue != NULL);
+ entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_VERSION;
+ entry->typed.versionValue = versionValue;
+ bool written = celix_version_fillString(versionValue,
convertedValueBuffer, sizeof(convertedValueBuffer));
+ if (written) {
+ entry->value = celix_properties_createString(properties,
convertedValueBuffer);
+ } else {
+ entry->value = celix_version_toString(versionValue);
+ }
+ }
+ if (entry->value == NULL) {
+ return CELIX_ENOMEM;
+ }
+ return CELIX_SUCCESS;
+}
+
+/**
+ * Allocate entry and optionally use the short properties optimization entries
buffer.
+ */
+celix_properties_entry_t* celix_properties_allocEntry(celix_properties_t*
properties) {
+ celix_properties_entry_t* entry;
+ if (properties->currentEntriesBufferIndex <
CELIX_SHORT_PROPERTIES_OPTIMIZATION_ENTRIES_SIZE) {
+ entry =
&properties->entriesBuffer[properties->currentEntriesBufferIndex++];
+ } else {
+ entry = malloc(sizeof(*entry));
+ }
+ return entry;
+}
+
+/**
+ * Create entry and optionally use the short properties optimization entries
buffer and take ownership of the
+ * provided key and value strings.
+ */
+static celix_properties_entry_t*
celix_properties_createEntryWithNoCopy(celix_properties_t* properties,
+ const
char* strValue) {
+ celix_properties_entry_t* entry = celix_properties_allocEntry(properties);
+ if (entry == NULL) {
+ return NULL;
+ }
+ entry->value = strValue;
+ entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_STRING;
+ entry->typed.strValue = strValue;
+ return entry;
+}
+
+/**
+ * Create entry and optionally use the short properties optimization buffers.
+ * Only 1 of the types values (strValue, LongValue, etc) should be provided.
+ */
+static celix_properties_entry_t*
celix_properties_createEntry(celix_properties_t* properties,
+ const char**
strValue,
+ const long*
longValue,
+ const double*
doubleValue,
+ const bool*
boolValue,
+ celix_version_t*
versionValue) {
+ celix_properties_entry_t* entry = celix_properties_allocEntry(properties);
+ if (entry == NULL) {
+ return NULL;
+ }
+
+ celix_status_t status =
+ celix_properties_fillEntry(properties, entry, strValue, longValue,
doubleValue, boolValue, versionValue);
+ if (status != CELIX_SUCCESS) {
+ if (entry >= properties->entriesBuffer &&
+ entry <= (properties->entriesBuffer +
CELIX_SHORT_PROPERTIES_OPTIMIZATION_ENTRIES_SIZE)) {
+ // entry is part of the properties entries buffer -> nop.
+ } else {
+ free(entry);
+ }
+ entry = NULL;
+ }
+ return entry;
+}
+
+static void celix_properties_destroyEntry(celix_properties_t* properties,
celix_properties_entry_t* entry) {
+ celix_properties_freeString(properties, (char*)entry->value);
+ if (entry->valueType == CELIX_PROPERTIES_VALUE_TYPE_VERSION) {
+ celix_version_destroy((celix_version_t*)entry->typed.versionValue);
+ }
+
+ if (entry >= properties->entriesBuffer &&
+ entry <= (properties->entriesBuffer +
CELIX_SHORT_PROPERTIES_OPTIMIZATION_ENTRIES_SIZE)) {
+ // entry is part of the properties entries buffer -> nop.
+ } else {
+ free(entry);
+ }
+}
+
+/**
+ * Create and add entry and optionally use the short properties optimization
buffers.
+ * Only 1 of the types values (strValue, LongValue, etc) should be provided.
+ */
+static celix_status_t celix_properties_createAndSetEntry(celix_properties_t*
properties,
+ const char* key,
+ const char** strValue,
+ const long* longValue,
+ const double*
doubleValue,
+ const bool* boolValue,
+ celix_version_t*
versionValue) {
+ if (!properties) {
+ return CELIX_SUCCESS; // silently ignore
+ }
+ if (!key) {
+ celix_err_push("Cannot set property with NULL key");
+ return CELIX_SUCCESS; // print error and ignore
Review Comment:
If `CELIX_SUCCESS` is returned, how the caller can know he/she needs to log
ERR?
##########
libs/utils/include/celix_string_hash_map.h:
##########
@@ -90,14 +94,37 @@ typedef struct celix_string_hash_map_create_options {
* only the simpledRemoveCallback will be used.
*
* Default is NULL.
+ *
+ * @param[in] data The void pointer to the data that was provided when the
callback was set as removedCallbackData.
+ * @param[in] removedKey The key of the value that was removed from the
hash map.
+ * Note that the removedKey can still be in use if
the a value entry for the same key is
+ * replaced, so this callback should not free the
removedKey.
+ * @param[in] removedValue The value that was removed from the hash map.
This value is no longer used by the
+ * hash map and can be freed.
*/
void (*removedCallback)(void* data, const char* removedKey,
celix_hash_map_value_t removedValue) CELIX_OPTS_INIT;
+ /**
+ * @brief A removed key callback, which if provided will be called if a
key is no longer used in the hash map.
+ *
+ * @param[in] data The void pointer to the data that was provided when the
callback was set as removedCallbackData.
+ * @param[in] key The key that is no longer used in the hash map. if
`storeKeysWeakly` was configured as true,
+ * the key can be freed.
+ */
+ void (*removedKeyCallback)(void* data, char* key) CELIX_OPTS_INIT;
+
/**
* @brief If set to true, the string hash map will not make of copy of the
keys and assumes
* that the keys are in scope/memory for the complete lifecycle of the
string hash map.
*
- * Note that this changes the default behaviour of the
celix_stringHashMap_put* functions.
+ * When keys are stored weakly it is the caller responsibility to check
the return value of
+ * celix_stringHashMap_put* function calls.
+ * If a celix_stringHashMap_put* function call returns true, the key is
used in the hash map and the key
Review Comment:
It no longer returns true/false.
##########
libs/utils/include/celix/Version.h:
##########
@@ -0,0 +1,175 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <memory>
+#include <string>
+
+#include "celix_version.h"
+
+namespace celix {
+
+ /**
+ * @class Version
+ * @brief Class for storing and manipulating version information.
+ *
+ * The Version class represents a version number that follows the Semantic
Versioning specification (SemVer).
+ * It consists of three non-negative integers for the major, minor, and
micro version, and an optional string for
+ * the qualifier.
+ * The Version class provides comparison operators and functions for
getting the individual version components.
+ *
+ * @note This class is a thin wrapper around the C API defined in
celix_version.h.
+ */
+ class Version {
+ public:
+
+ ///@brief Constructs a new empty version with all components set to
zero.
+ Version() :
+ cVersion{createVersion(celix_version_createEmptyVersion())},
+ qualifier{celix_version_getQualifier(cVersion.get())} {}
+
+ /**
+ * @brief Constructs a new version with the given components and
qualifier.
+ * @param major The major component of the version.
+ * @param minor The minor component of the version.
+ * @param micro The micro component of the version.
+ * @param qualifier The qualifier string of the version.
+ */
+#if __cplusplus >= 201703L //C++17 or higher
+ Version(int major, int minor, int micro, std::string_view qualifier =
{}) :
+ cVersion{createVersion(celix_version_create(major, minor, micro,
qualifier.empty() ? "" : qualifier.data()))},
+ qualifier{celix_version_getQualifier(cVersion.get())} {}
+#else
+ explicit Version(int major, int minor, int micro, const std::string&
qualifier = {}) :
+ cVersion{createVersion(celix_version_create(major, minor, micro,
qualifier.empty() ? "" : qualifier.c_str()))},
+ qualifier{celix_version_getQualifier(cVersion.get())} {}
+#endif
+
+ ///@brief Move-constructs a new version from an existing one.
+ Version(Version&&) = default;
Review Comment:
What's the status of `cVersion` after move? Will this break class invariant
(and lead to crash)?
##########
libs/utils/include/celix_string_hash_map.h:
##########
@@ -90,14 +94,37 @@ typedef struct celix_string_hash_map_create_options {
* only the simpledRemoveCallback will be used.
*
* Default is NULL.
+ *
+ * @param[in] data The void pointer to the data that was provided when the
callback was set as removedCallbackData.
+ * @param[in] removedKey The key of the value that was removed from the
hash map.
+ * Note that the removedKey can still be in use if
the a value entry for the same key is
+ * replaced, so this callback should not free the
removedKey.
+ * @param[in] removedValue The value that was removed from the hash map.
This value is no longer used by the
+ * hash map and can be freed.
*/
void (*removedCallback)(void* data, const char* removedKey,
celix_hash_map_value_t removedValue) CELIX_OPTS_INIT;
+ /**
+ * @brief A removed key callback, which if provided will be called if a
key is no longer used in the hash map.
+ *
+ * @param[in] data The void pointer to the data that was provided when the
callback was set as removedCallbackData.
+ * @param[in] key The key that is no longer used in the hash map. if
`storeKeysWeakly` was configured as true,
+ * the key can be freed.
+ */
+ void (*removedKeyCallback)(void* data, char* key) CELIX_OPTS_INIT;
+
/**
* @brief If set to true, the string hash map will not make of copy of the
keys and assumes
* that the keys are in scope/memory for the complete lifecycle of the
string hash map.
*
- * Note that this changes the default behaviour of the
celix_stringHashMap_put* functions.
+ * When keys are stored weakly it is the caller responsibility to check
the return value of
+ * celix_stringHashMap_put* function calls.
+ * If a celix_stringHashMap_put* function call returns true, the key is
used in the hash map and the key
+ * should never be freed or freed in a configured removedKeyCallback.
+ * If a celix_stringHashMap_put* function call returns false, a value is
replaced and the already existing
+ * key is reused. If the needed the caller should free the provided key.
Review Comment:
This consideration also applies to `celix_stringHashMap_remove` and
`celix_stringHashMap_clear`.
##########
libs/utils/include/celix_string_hash_map.h:
##########
@@ -90,14 +94,37 @@ typedef struct celix_string_hash_map_create_options {
* only the simpledRemoveCallback will be used.
*
* Default is NULL.
+ *
+ * @param[in] data The void pointer to the data that was provided when the
callback was set as removedCallbackData.
+ * @param[in] removedKey The key of the value that was removed from the
hash map.
+ * Note that the removedKey can still be in use if
the a value entry for the same key is
+ * replaced, so this callback should not free the
removedKey.
+ * @param[in] removedValue The value that was removed from the hash map.
This value is no longer used by the
+ * hash map and can be freed.
*/
void (*removedCallback)(void* data, const char* removedKey,
celix_hash_map_value_t removedValue) CELIX_OPTS_INIT;
+ /**
+ * @brief A removed key callback, which if provided will be called if a
key is no longer used in the hash map.
+ *
+ * @param[in] data The void pointer to the data that was provided when the
callback was set as removedCallbackData.
+ * @param[in] key The key that is no longer used in the hash map. if
`storeKeysWeakly` was configured as true,
+ * the key can be freed.
+ */
+ void (*removedKeyCallback)(void* data, char* key) CELIX_OPTS_INIT;
+
/**
* @brief If set to true, the string hash map will not make of copy of the
keys and assumes
* that the keys are in scope/memory for the complete lifecycle of the
string hash map.
*
- * Note that this changes the default behaviour of the
celix_stringHashMap_put* functions.
+ * When keys are stored weakly it is the caller responsibility to check
the return value of
+ * celix_stringHashMap_put* function calls.
+ * If a celix_stringHashMap_put* function call returns true, the key is
used in the hash map and the key
+ * should never be freed or freed in a configured removedKeyCallback.
+ * If a celix_stringHashMap_put* function call returns false, a value is
replaced and the already existing
+ * key is reused. If the needed the caller should free the provided key.
Review Comment:
Then what shall we do if the already existing key is reused? Now we have no
way to know that. We could require removeKeyCallback to be set if keys are
stored weakly, and trigger the callback to notify the caller in this case. Will
it work?
--
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
