This is an automated email from the ASF dual-hosted git repository. pnoltes pushed a commit to branch feature/type_support_for_properties in repository https://gitbox.apache.org/repos/asf/celix.git
commit 02b08a110d221aab7f65bf14b3af7d4d48692fb0 Author: Pepijn Noltes <[email protected]> AuthorDate: Tue Jan 3 22:55:24 2023 +0100 Improve celix properties iterator --- libs/utils/gtest/src/PropertiesTestSuite.cc | 61 +++++++++++- libs/utils/include/celix/Properties.h | 26 +++-- libs/utils/include/celix_long_hash_map.h | 9 +- libs/utils/include/celix_properties.h | 145 ++++++++++++++-------------- libs/utils/include/celix_string_hash_map.h | 6 +- libs/utils/src/properties.c | 15 +-- 6 files changed, 158 insertions(+), 104 deletions(-) diff --git a/libs/utils/gtest/src/PropertiesTestSuite.cc b/libs/utils/gtest/src/PropertiesTestSuite.cc index b2980282..1528ddb2 100644 --- a/libs/utils/gtest/src/PropertiesTestSuite.cc +++ b/libs/utils/gtest/src/PropertiesTestSuite.cc @@ -233,6 +233,20 @@ TEST_F(PropertiesTestSuite, boolTest) { celix_properties_destroy(properties); } +TEST_F(PropertiesTestSuite, fillTest) { + celix_properties_t *props = celix_properties_create(); + int testCount = 1000; + for (int i = 0; i < 1000; ++i) { + char k[5]; + snprintf(k, sizeof(k), "%i", i); + const char* v = "a"; + celix_properties_set(props, k, v); + } + EXPECT_EQ(celix_properties_size(props), testCount); + celix_properties_destroy(props); +} + + TEST_F(PropertiesTestSuite, sizeAndIteratorTest) { celix_properties_t *props = celix_properties_create(); EXPECT_EQ(0, celix_properties_size(props)); @@ -274,12 +288,12 @@ TEST_F(PropertiesTestSuite, getType) { } TEST_F(PropertiesTestSuite, getEntry) { - auto *props = celix_properties_create(); + auto* props = celix_properties_create(); celix_properties_set(props, "key1", "value1"); celix_properties_setLong(props, "key2", 123); celix_properties_setDouble(props, "key3", 123.456); celix_properties_setBool(props, "key4", true); - auto *version = celix_version_createVersion(1, 2, 3, nullptr); + auto* version = celix_version_createVersion(1, 2, 3, nullptr); celix_properties_setVersion(props, "key5", version); auto entry = celix_properties_getEntry(props, "key1"); @@ -316,7 +330,7 @@ TEST_F(PropertiesTestSuite, getEntry) { } TEST_F(PropertiesTestSuite, iteratorNextKey) { - auto *props = celix_properties_create(); + auto* props = celix_properties_create(); celix_properties_set(props, "key1", "value1"); celix_properties_set(props, "key2", "value2"); celix_properties_set(props, "key3", "value3"); @@ -336,7 +350,7 @@ TEST_F(PropertiesTestSuite, iteratorNextKey) { } TEST_F(PropertiesTestSuite, iteratorNext) { - auto *props = celix_properties_create(); + auto* props = celix_properties_create(); celix_properties_set(props, "key1", "value1"); celix_properties_set(props, "key2", "value2"); celix_properties_set(props, "key3", "value3"); @@ -357,7 +371,7 @@ TEST_F(PropertiesTestSuite, iteratorNext) { } TEST_F(PropertiesTestSuite, iterateOverProperties) { - celix_properties_t* props = celix_properties_create(); + auto* props = celix_properties_create(); celix_properties_set(props, "key1", "value1"); celix_properties_set(props, "key2", "value2"); @@ -378,4 +392,41 @@ TEST_F(PropertiesTestSuite, iterateOverProperties) { EXPECT_EQ(innerCount, 4); celix_properties_destroy(props); + + + props = celix_properties_create(); + int count = 0; + CELIX_PROPERTIES_ITERATE(props, outerIter) { + count++; + } + EXPECT_EQ(count, 0); + celix_properties_destroy(props); +} + +TEST_F(PropertiesTestSuite, getAsVersion) { + auto* properties = celix_properties_create(); + + // Test getting a version property + auto* expected = celix_version_createVersion(1, 2, 3, "test"); + celix_properties_setVersion(properties, "key", expected); + const auto* actual = celix_properties_getAsVersion(properties, "key", nullptr); + EXPECT_EQ(celix_version_getMajor(expected), celix_version_getMajor(actual)); + EXPECT_EQ(celix_version_getMinor(expected), celix_version_getMinor(actual)); + EXPECT_EQ(celix_version_getMicro(expected), celix_version_getMicro(actual)); + EXPECT_STREQ(celix_version_getQualifier(expected), celix_version_getQualifier(actual)); + + // Test getting a non-version property + celix_properties_set(properties, "key2", "value"); + auto* emptyVersion = celix_version_createEmptyVersion(); + actual = celix_properties_getAsVersion(properties, "key2", emptyVersion); + EXPECT_EQ(celix_version_getMajor(actual), 0); + EXPECT_EQ(celix_version_getMinor(actual), 0); + EXPECT_EQ(celix_version_getMicro(actual), 0); + EXPECT_STREQ(celix_version_getQualifier(actual), ""); + + EXPECT_EQ(celix_properties_getAsVersion(properties, "non-existent", nullptr), nullptr); + + celix_version_destroy(expected); + celix_version_destroy(emptyVersion); + celix_properties_destroy(properties); } \ No newline at end of file diff --git a/libs/utils/include/celix/Properties.h b/libs/utils/include/celix/Properties.h index da653616..111c7fe4 100644 --- a/libs/utils/include/celix/Properties.h +++ b/libs/utils/include/celix/Properties.h @@ -34,14 +34,8 @@ namespace celix { */ class PropertiesIterator { public: - explicit PropertiesIterator(celix_properties_t* props) { - iter = celix_propertiesIterator_construct(props); - next(); - } - explicit PropertiesIterator(const celix_properties_t* props) { - iter = celix_propertiesIterator_construct(props); - next(); + iter = celix_properties_begin(props); } PropertiesIterator& operator++() { @@ -65,17 +59,17 @@ namespace celix { } void next() { - if (celix_propertiesIterator_hasNext(&iter)) { - auto* k = celix_propertiesIterator_nextKey(&iter); - auto* props = celix_propertiesIterator_properties(&iter); - auto *v = celix_properties_get(props, k, ""); - first = std::string{(const char*)k}; - second = std::string{(const char*)v}; - } else { + celix_propertiesIterator_next(&iter); + if (celix_propertiesIterator_isEnd(&iter)) { moveToEnd(); + } else { + first = iter.entry.key; + second = iter.entry.value; + end = false; } } + //TODO try to remove moveToEnd void moveToEnd() { first = {}; second = {}; @@ -85,6 +79,7 @@ namespace celix { //TODO for C++17 try to update first and second to stringview std::string first{}; std::string second{}; + //TODO iter? private: celix_properties_iterator_t iter{.index = -1, .entry = {}, ._data = {}}; bool end{false}; @@ -135,6 +130,8 @@ namespace celix { } #endif + //TODO operator= with long, double, boolean and version + [[nodiscard]] const char* getValue() const { if (charKey == nullptr) { return celix_properties_get(props.get(), stringKey.c_str(), nullptr); @@ -306,6 +303,7 @@ namespace celix { std::string_view view{value}; celix_properties_set(cProps.get(), key.data(), view.data()); } else { + //TODO spit up to use setLong, setBool, setDouble and setVersion using namespace std; celix_properties_set(cProps.get(), key.data(), to_string(value).c_str()); } diff --git a/libs/utils/include/celix_long_hash_map.h b/libs/utils/include/celix_long_hash_map.h index c704ab60..50bf810d 100644 --- a/libs/utils/include/celix_long_hash_map.h +++ b/libs/utils/include/celix_long_hash_map.h @@ -261,8 +261,10 @@ bool celix_longHashMap_remove(celix_long_hash_map_t* map, long key); void celix_longHashMap_clear(celix_long_hash_map_t* map); /** - * @brief Create and return a hash map iterator for the beginning of the hash map. + * @brief Returns an iterator pointing to the first element in the map. * + * @param map The map to get the iterator for. + * @return An iterator pointing to the first element in the map. */ celix_long_hash_map_iterator_t celix_longHashMap_begin(const celix_long_hash_map_t* map); @@ -284,7 +286,7 @@ void celix_longHashMapIterator_next(celix_long_hash_map_iterator_t* iter); * @brief Marco to loop over all the entries of a long hash map. * * Small example of how to use the iterate macro: - * @code + * @code{.c} * celix_long_hash_map_t* map = ... * CELIX_LONG_HASH_MAP_ITERATE(map, iter) { * printf("Visiting hash map entry with key %li\n", inter.key); @@ -294,7 +296,6 @@ void celix_longHashMapIterator_next(celix_long_hash_map_iterator_t* iter); * @param map The (const celix_long_hash_map_t*) map to iterate over. * @param iterName A iterName which will be of type celix_long_hash_map_iterator_t to hold the iterator. */ -//TODO test if the macro can be used nested #define CELIX_LONG_HASH_MAP_ITERATE(map, iterName) \ for (celix_long_hash_map_iterator_t iterName = celix_longHashMap_begin(map); !celix_longHashMapIterator_isEnd(&(iterName)); celix_longHashMapIterator_next(&(iterName))) @@ -302,7 +303,7 @@ void celix_longHashMapIterator_next(celix_long_hash_map_iterator_t* iter); * @brief Remove the hash map entry for the provided iterator and updates the iterator to the next hash map entry * * Small example of how to use the celix_longHashMapIterator_remove function: - * @code + * @code{.c} * //remove all even entries * celix_long_hash_map_t* map = ... * celix_long_hash_map_iterator_t iter = celix_longHashMap_begin(map); diff --git a/libs/utils/include/celix_properties.h b/libs/utils/include/celix_properties.h index 3c876697..45846506 100644 --- a/libs/utils/include/celix_properties.h +++ b/libs/utils/include/celix_properties.h @@ -43,6 +43,8 @@ extern "C" { /** * @brief celix_properties_t is a type that represents a set of key-value pairs called properties, * which can be used to store configuration data or metadata for a services, components or framework configuration. + * + * @note Not thread safe. */ typedef struct celix_properties celix_properties_t; @@ -293,15 +295,13 @@ void celix_properties_setVersion(celix_properties_t* properties, const char* key /** * @brief Gets the value of a property as a Celix version. * - * //TODO, maybe improve, now returns NULL if underlining type is not version + * This function does not convert a string property value to a Celix version automatically. * - * @param properties The property set to search. - * @param key The key of the property to get. - * @param defaultValue The value to return if the property is not set, the value is not a Celix version, or if - * the value cannot be converted to a Celix version. - * @return The value of the property as a Celix version, or the default value if the property is not set, - * the value is not a Celix version, or if the value cannot be converted to a Celix version. - * If the value is a string, it will be verified as a valid Celix version. + * @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 The value of the property if it is a Celix version, or the default value if the property is not set or the + * value is not a Celix version. */ const celix_version_t* celix_properties_getAsVersion( const celix_properties_t* properties, @@ -316,41 +316,6 @@ const celix_version_t* celix_properties_getAsVersion( */ int celix_properties_size(const celix_properties_t* properties); -/** - * @brief Constructs a new properties iterator. - * - * Note: The iterator is initialized to be before the first entry. To advance to the first entry, - * call `celix_propertiesIterator_nextEntry`. - * - * //TODO deprecate? - * - * @param[in] properties The properties object to iterate over. - * @return The newly constructed iterator. - */ -celix_properties_iterator_t celix_propertiesIterator_construct(const celix_properties_t* properties); - -/** - * @brief Determines whether the iterator has more entries. - * - * //TODO deprecate? - * - * @param[in] iter The iterator. - * @return true if the iterator has more entries, false otherwise. - */ -bool celix_propertiesIterator_hasNext(celix_properties_iterator_t *iter); - -/** - * @brief Advances the iterator to the next entry and returns the key for the current entry. - * - * If the iterator has no more entries, this function returns NULL. - * - * //TODO deprecate? - * - * @param[in, out] iter The iterator. - * @return The key for the current entry, or NULL if the iterator has no more entries. - */ -const char* celix_propertiesIterator_nextKey(celix_properties_iterator_t* iter); - /** * @brief Constructs an iterator pointing to the first entry in the properties object. * @@ -393,13 +358,76 @@ celix_properties_t* celix_propertiesIterator_properties(const celix_properties_i */ bool celix_propertiesIterator_equals(const celix_properties_iterator_t* a, const celix_properties_iterator_t* b); +/** + * @brief Iterates over the entries in the specified celix_properties_t object. + * + * This macro allows you to easily iterate over the entries in a celix_properties_t object. + * The loop variable `iterName` will be of type celix_properties_iterator_t and will contain the current + * entry during each iteration. + * + * @param map The properties object to iterate over. + * @param iterName The name of the iterator variable to use in the loop. + * + * Example usage: + * @code{.c} + * // Iterate over all entries in the properties object + * CELIX_PROPERTIES_ITERATE(properties, iter) { + * // Print the key and value of the current entry + * printf("%s: %s\n", iter.entry.key, iter.entry.value); + * } + * @endcode + */ +#define CELIX_PROPERTIES_ITERATE(map, iterName) \ + for (celix_properties_iterator_t iterName = celix_properties_begin(map); \ + !celix_propertiesIterator_isEnd(&(iterName)); celix_propertiesIterator_next(&(iterName))) + + + + +/**** Deprecated API *************************************************************************************************/ + +/** + * @brief Constructs a new properties iterator. + * @deprecated This function is deprecated, use celix_properties_begin, celix_propertiesIterator_next and celix_propertiesIterator_isEnd instead. + * + * Note: The iterator is initialized to be before the first entry. To advance to the first entry, + * call `celix_propertiesIterator_nextEntry`. + * + * @param[in] properties The properties object to iterate over. + * @return The newly constructed iterator. + */ +celix_properties_iterator_t celix_propertiesIterator_construct(const celix_properties_t* properties) + __attribute__((deprecated("celix_propertiesIterator_construct is deprecated use celix_properties_begin, celix_propertiesIterator_next and celix_propertiesIterator_isEnd instead"))); + +/** + * @brief Determines whether the iterator has more entries. + * @deprecated This function is deprecated, use celix_properties_begin, celix_propertiesIterator_next and celix_propertiesIterator_isEnd instead. + * + * @param[in] iter The iterator. + * @return true if the iterator has more entries, false otherwise. + */ +bool celix_propertiesIterator_hasNext(celix_properties_iterator_t *iter) + __attribute__((deprecated("celix_propertiesIterator_hasNext is deprecated use celix_properties_begin, celix_propertiesIterator_next and celix_propertiesIterator_isEnd instead"))); + +/** + * @brief Advances the iterator to the next entry and returns the key for the current entry. + * @deprecated This function is deprecated, use celix_properties_begin, celix_propertiesIterator_next and celix_propertiesIterator_isEnd instead. + * + * If the iterator has no more entries, this function returns NULL. + * + * @param[in, out] iter The iterator. + * @return The key for the current entry, or NULL if the iterator has no more entries. + */ +const char* celix_propertiesIterator_nextKey(celix_properties_iterator_t* iter) + __attribute__((deprecated("celix_propertiesIterator_nextKey is deprecated use celix_properties_begin, celix_propertiesIterator_next and celix_propertiesIterator_isEnd instead"))); + /** * @brief Macro for iterating over the properties in a property set. + * @deprecated This macro is deprecated, use CELIX_PROPERTIES_ITERATE instead. * * @param[in] properties The property set to iterate over. * @param[out] key The variable to use for the current key in the loop. * - * //TODO deprecate * * Example usage: * @code{.c} @@ -421,40 +449,11 @@ bool celix_propertiesIterator_equals(const celix_properties_iterator_t* a, const * key3 = value3 * @endcode */ -#define CELIX_PROPERTIES_FOR_EACH(map, keyName) \ - for (celix_properties_iterator_t iter_##keyName = celix_properties_begin(map); \ - (keyName) = iter_##keyName.entry.key, !celix_propertiesIterator_isEnd(&(iter_##keyName)); \ - celix_propertiesIterator_next(&(iter_##keyName))) - -/* #define CELIX_PROPERTIES_FOR_EACH(properties, key) \ for(celix_properties_iterator_t iter_##key = celix_propertiesIterator_construct(properties); \ celix_propertiesIterator_hasNext(&iter_##key), (key) = celix_propertiesIterator_nextKey(&iter_##key);) -*/ -/** - * @brief Iterates over the entries in the specified celix_properties_t object. - * - * This macro allows you to easily iterate over the entries in a celix_properties_t object. - * The loop variable `iterName` will be of type celix_properties_iterator_t and will contain the current - * entry during each iteration. - * - * @param map The properties object to iterate over. - * @param iterName The name of the iterator variable to use in the loop. - * - * Example usage: - * @code{.c} - * // Iterate over all entries in the properties object - * CELIX_PROPERTIES_ITERATE(properties, iter) { - * // Print the key and value of the current entry - * printf("%s: %s\n", iter.entry.key, iter.entry.value); - * } - * @endcode - */ -#define CELIX_PROPERTIES_ITERATE(map, iterName) \ - for (celix_properties_iterator_t iterName = celix_properties_begin(map); \ - !celix_propertiesIterator_isEnd(&(iterName)); celix_propertiesIterator_next(&(iterName))) #ifdef __cplusplus } diff --git a/libs/utils/include/celix_string_hash_map.h b/libs/utils/include/celix_string_hash_map.h index 97c1735b..1e5fbe82 100644 --- a/libs/utils/include/celix_string_hash_map.h +++ b/libs/utils/include/celix_string_hash_map.h @@ -270,7 +270,10 @@ bool celix_stringHashMap_remove(celix_string_hash_map_t* map, const char* key); void celix_stringHashMap_clear(celix_string_hash_map_t* map); /** - * @brief Create and return a hash map iterator for the beginning of the hash map. + * @brief Returns an iterator pointing to the first element in the map. + * + * @param map The map to get the iterator for. + * @return An iterator pointing to the first element in the map. */ celix_string_hash_map_iterator_t celix_stringHashMap_begin(const celix_string_hash_map_t* map); @@ -303,7 +306,6 @@ void celix_stringHashMapIterator_next(celix_string_hash_map_iterator_t* iter); * @param map The (const celix_string_hash_map_t*) map to iterate over. * @param iterName A iterName which will be of type celix_string_hash_map_iterator_t to hold the iterator. */ -//TODO test if the macro can be used nested #define CELIX_STRING_HASH_MAP_ITERATE(map, iterName) \ for (celix_string_hash_map_iterator_t iterName = celix_stringHashMap_begin(map); !celix_stringHashMapIterator_isEnd(&(iterName)); celix_stringHashMapIterator_next(&(iterName))) diff --git a/libs/utils/src/properties.c b/libs/utils/src/properties.c index af8b892f..9185fa64 100644 --- a/libs/utils/src/properties.c +++ b/libs/utils/src/properties.c @@ -387,6 +387,9 @@ static void celix_properties_createAndSetEntry( const double* doubleValue, const bool* boolValue, const celix_version_t* versionValue) { + if (properties == NULL) { + return; + } celix_properties_entry_t* entry = celix_properties_createEntry(properties, key, strValue, longValue, doubleValue, boolValue, versionValue); if (entry != NULL) { @@ -696,12 +699,8 @@ const celix_version_t* celix_properties_getAsVersion( celix_properties_entry_t* entry = celix_stringHashMap_get(properties->map, key); if (entry != NULL && entry->valueType == CELIX_PROPERTIES_VALUE_TYPE_VERSION) { return entry->typed.versionValue; - } else if (entry != NULL) { - //NOTE not converting to version, due to ownership - //TODO improve? - return NULL; } - return NULL; + return defaultValue; } void celix_properties_setVersion(celix_properties_t *props, const char *key, const celix_version_t* version) { @@ -767,7 +766,11 @@ celix_properties_iterator_t celix_properties_begin(const celix_properties_t* pro celix_properties_iterator_t iter; iter.index = 0; - memcpy(&iter.entry, internalIter.mapIter.value.ptrValue, sizeof(iter.entry)); + if (!celix_stringHashMapIterator_isEnd(&internalIter.mapIter)) { + memcpy(&iter.entry, internalIter.mapIter.value.ptrValue, sizeof(iter.entry)); + } else { + memset(&iter.entry, 0, sizeof(iter.entry)); + } memset(&iter._data, 0, sizeof(iter._data)); memcpy(iter._data, &internalIter, sizeof(internalIter));
