PengZheng commented on code in PR #470:
URL: https://github.com/apache/celix/pull/470#discussion_r1378446643
##########
libs/rcm/src/celix_capability.c:
##########
@@ -83,22 +83,19 @@ bool celix_capability_equals(const celix_capability_t*
cap1, const celix_capabil
//compare attributes
bool equals = true;
- const char* visit;
- CELIX_PROPERTIES_FOR_EACH(cap1->attributes, visit) {
- const char* value1 = celix_properties_get(cap1->attributes, visit,
NULL);
- const char* value2 = celix_properties_get(cap2->attributes, visit,
NULL);
- if (!celix_utils_stringEquals(value1, value2)) {
+ CELIX_PROPERTIES_ITERATE(cap1->attributes, visit) {
Review Comment:
Shall we use `celix_properties_equals` instead?
##########
libs/utils/gtest/src/CxxPropertiesTestSuite.cc:
##########
@@ -74,17 +97,111 @@ TEST_F(CxxPropertiesTestSuite, testCopy) {
EXPECT_EQ(v2, "value1_new");
}
-TEST_F(CxxPropertiesTestSuite, testWrap) {
+TEST_F(CxxPropertiesTestSuite, WrapTest) {
auto *props = celix_properties_create();
celix_properties_set(props, "test", "test");
EXPECT_EQ(1, celix_properties_size(props));
{
auto cxxProps = celix::Properties::wrap(props);
- EXPECT_EQ(1, cxxProps->size());
- EXPECT_EQ(props, cxxProps->getCProperties());
+ EXPECT_EQ(1, cxxProps.size());
+ EXPECT_EQ(props, cxxProps.getCProperties());
+ } //NOTE cxxProps out of scope, but will not destroy celix_properties
+ EXPECT_EQ(1, celix_properties_size(props));
+
+ celix_properties_destroy(props);
+}
+
+TEST_F(CxxPropertiesTestSuite, CopyCPropsTest) {
+ auto *props = celix_properties_create();
+ celix_properties_set(props, "test", "test");
+
+ EXPECT_EQ(1, celix_properties_size(props));
+ {
+ auto cxxProps = celix::Properties::copy(props);
+ EXPECT_EQ(1, cxxProps.size());
+ EXPECT_NE(props, cxxProps.getCProperties());
} //NOTE cxxProps out of scope, but will not destroy celix_properties
EXPECT_EQ(1, celix_properties_size(props));
celix_properties_destroy(props);
}
+
+TEST_F(CxxPropertiesTestSuite, GetTypeTest) {
+ celix::Properties props{};
+
+ const auto v2 = celix::Version{1, 2, 3};
+ auto v3 = celix::Version{1, 2, 4};
+
+ props.set("bool", true);
+ props.set("long1", 1l);
+ props.set("long2", (int)1); //should lead to long;
+ props.set("long3", (unsigned int)1); //should lead to long;
+ props.set("long4", (short)1); //should lead to long;
+ props.set("long5", (unsigned short)1); //should lead to long;
+ props.set("long6", (char)1); //should lead to long;
+ props.set("long7", (unsigned char)1); //should lead to long;
+ props.set("double1", 1.0);
+ props.set("double2", 1.0f); //set float should lead to double
+ props.set("version1", celix::Version{1, 2, 3});
+ props.set("version2", v2);
+ props.set("version3", v3);
+
+ EXPECT_EQ(props.getType("bool"), celix::Properties::ValueType::Bool);
+ EXPECT_EQ(props.getType("long1"), celix::Properties::ValueType::Long);
+ EXPECT_EQ(props.getType("long2"), celix::Properties::ValueType::Long);
+ EXPECT_EQ(props.getType("long3"), celix::Properties::ValueType::Long);
+ EXPECT_EQ(props.getType("long4"), celix::Properties::ValueType::Long);
+ EXPECT_EQ(props.getType("long5"), celix::Properties::ValueType::Long);
+ EXPECT_EQ(props.getType("long6"), celix::Properties::ValueType::Long);
+ EXPECT_EQ(props.getType("long7"), celix::Properties::ValueType::Long);
+ EXPECT_EQ(props.getType("double1"), celix::Properties::ValueType::Double);
+ EXPECT_EQ(props.getType("double2"), celix::Properties::ValueType::Double);
+ EXPECT_EQ(props.getType("version1"),
celix::Properties::ValueType::Version);
+ EXPECT_EQ(props.getType("version2"),
celix::Properties::ValueType::Version);
+ EXPECT_EQ(props.getType("version3"),
celix::Properties::ValueType::Version);
+}
+
+TEST_F(CxxPropertiesTestSuite, GetAsVersionTest) {
+ celix::Properties props;
+
+ // Test getting a version from a string property
+ props.set("key", "1.2.3");
+ celix::Version ver{1, 2, 3};
+ EXPECT_TRUE(props.getAsVersion("key") == ver);
+
+ // Test getting a version from a version property
+ props.set("key", celix::Version{2, 3, 4});
+ ver = celix::Version{2, 3, 4};
+ EXPECT_EQ(props.getAsVersion("key"), ver);
+
+ // Test getting default value when property is not set
+ ver = celix::Version{3, 4, 5};
+ EXPECT_EQ(props.getAsVersion("non_existent_key", celix::Version{3, 4, 5}),
ver);
+
+ // Test getting default value when property value is not a valid version
string
+ props.set("key", "invalid_version_string");
+ ver = celix::Version{4, 5, 6};
+ EXPECT_EQ(props.getAsVersion("key", celix::Version{4, 5, 6}), ver);
+}
+
+TEST_F(CxxPropertiesTestSuite, StoreAndLoadTest) {
+ std::string path{"cxx_store_and_load_test.properties"};
+
+ celix::Properties props{};
+ props.set("key1", 1);
+ props.set("key2", 2);
+
+ EXPECT_NO_THROW(props.store(path));
+
+ celix::Properties loadedProps{};
+ EXPECT_NO_THROW(loadedProps = celix::Properties::load(path));
+ EXPECT_EQ(props.size(), loadedProps.size());
Review Comment:
If #674 were solved, this should be equality test.
##########
libs/rcm/src/celix_requirement.c:
##########
@@ -89,45 +89,33 @@ bool celix_requirement_equals(const celix_requirement_t*
req1, const celix_requi
}
//compare attributes
- bool equals = true;
- const char* visit;
- CELIX_PROPERTIES_FOR_EACH(req1->attributes, visit) {
- const char* val1 = celix_properties_get(req1->attributes, visit, NULL);
- const char* val2 = celix_properties_get(req2->attributes, visit, NULL);
- if (!celix_utils_stringEquals(val1, val2)) {
- equals = false;
- break;
+ CELIX_PROPERTIES_ITERATE(req1->attributes, visit) {
Review Comment:
Shall we use `celix_properties_equals` instead?
##########
libs/utils/gtest/src/HashMapTestSuite.cc:
##########
@@ -534,25 +514,159 @@ TEST_F(HashMapTestSuite, IterateWithRemoveTest) {
auto iter2 = celix_longHashMap_begin(lMap);
while (!celix_longHashMapIterator_isEnd(&iter2)) {
if (iter2.index % 2 == 0) {
- //note only removing entries where the iter index is even
+ // note only removing entries where the iter index is even
celix_longHashMapIterator_remove(&iter2);
} else {
celix_longHashMapIterator_next(&iter2);
}
}
EXPECT_EQ(3, celix_longHashMap_size(lMap));
EXPECT_TRUE(celix_longHashMapIterator_isEnd(&iter2));
- celix_longHashMapIterator_next(&iter2); //calling next on end iter, does
nothing
+ celix_longHashMapIterator_next(&iter2); // calling next on end iter, does
nothing
EXPECT_TRUE(celix_longHashMapIterator_isEnd(&iter2));
celix_longHashMap_destroy(lMap);
}
-TEST_F(HashMapTestSuite, StringHashMapCleanup) {
+TEST_F(HashMapTestSuite, IterateEndTest) {
+ auto* sMap1 = createStringHashMap(0);
+ auto* sMap2 = createStringHashMap(6);
+ auto* lMap1 = createLongHashMap(0);
+ auto* lMap2 = createLongHashMap(6);
+
+ auto sIter1 = celix_stringHashMap_end(sMap1);
+ auto sIter2 = celix_stringHashMap_end(sMap2);
+ auto lIter1 = celix_longHashMap_end(lMap1);
+ auto lIter2 = celix_longHashMap_end(lMap2);
+
+ EXPECT_EQ(sIter1.index, 0);
+ EXPECT_EQ(sIter2.index, 6);
+ EXPECT_EQ(lIter1.index, 0);
+ EXPECT_EQ(lIter2.index, 6);
+ EXPECT_TRUE(celix_stringHashMapIterator_isEnd(&sIter1));
+ EXPECT_TRUE(celix_stringHashMapIterator_isEnd(&sIter2));
+ EXPECT_TRUE(celix_longHashMapIterator_isEnd(&lIter1));
+ EXPECT_TRUE(celix_longHashMapIterator_isEnd(&lIter2));
+
+ celix_stringHashMap_destroy(sMap1);
+ celix_stringHashMap_destroy(sMap2);
+ celix_longHashMap_destroy(lMap1);
+ celix_longHashMap_destroy(lMap2);
+}
+
+TEST_F(HashMapTestSuite, EqualsTest) {
+ auto* sMap = createStringHashMap(2);
+ auto sIter1 = celix_stringHashMap_begin(sMap);
+ auto sIter2 = celix_stringHashMap_begin(sMap);
+
+ // Test equal iterators
+ EXPECT_TRUE(celix_stringHashMapIterator_equals(&sIter1, &sIter2));
+
+ // Test unequal iterators after only 1 modification
+ celix_stringHashMapIterator_next(&sIter1);
+ EXPECT_FALSE(celix_stringHashMapIterator_equals(&sIter1, &sIter2));
+
+ // Test equal iterators after both modification
+ celix_stringHashMapIterator_next(&sIter2);
+ EXPECT_TRUE(celix_stringHashMapIterator_equals(&sIter1, &sIter2));
+
+ // Same for long hash map
+ auto* lMap = createLongHashMap(1);
+ auto lIter1 = celix_longHashMap_begin(lMap);
+ auto lIter2 = celix_longHashMap_begin(lMap);
+
+ // Test equal iterators
+ EXPECT_TRUE(celix_longHashMapIterator_equals(&lIter1, &lIter2));
+
+ // Test unequal iterators after only 1 modification
+ celix_longHashMapIterator_next(&lIter1);
+ EXPECT_FALSE(celix_longHashMapIterator_equals(&lIter1, &lIter2));
+
+ // Test equal iterators after both modification
+ celix_longHashMapIterator_next(&lIter2);
+ EXPECT_TRUE(celix_longHashMapIterator_equals(&lIter1, &lIter2));
+
+ celix_stringHashMap_destroy(sMap);
+ celix_longHashMap_destroy(lMap);
+}
+
+TEST_F(HashMapTestSuite, EqualsZeroSizeMapTest) {
+ // Because map size is 0, begin iter should equal end iter
+ auto* sMap = createStringHashMap(0);
+ auto sIter1 = celix_stringHashMap_begin(sMap);
+ auto sEnd = celix_stringHashMap_end(sMap);
+ EXPECT_TRUE(celix_stringHashMapIterator_equals(&sIter1, &sEnd));
+
+ // Because map size is 0, begin iter should equal end iter
+ auto* lMap = createLongHashMap(0);
+ auto lIter1 = celix_longHashMap_begin(lMap);
+ auto lEnd = celix_longHashMap_end(lMap);
+ EXPECT_TRUE(celix_longHashMapIterator_equals(&lIter1, &lEnd));
+
+ celix_stringHashMap_destroy(sMap);
+ celix_longHashMap_destroy(lMap);
+}
+
+TEST_F(HashMapTestSuite, StoreKeysWeaklyTest) {
+ celix_string_hash_map_create_options_t opts{};
+ opts.removedCallbackData = (void*)0x1;
+ opts.storeKeysWeakly = true;
+ opts.removedKeyCallback = [](void* data, char* key) {
+ EXPECT_EQ(data, (void*)0x1);
+ free(key);
+ };
+ auto* sMap = celix_stringHashMap_createWithOptions(&opts);
+ EXPECT_FALSE(celix_stringHashMap_putLong(sMap, celix_utils_strdup("key1"),
1)); // new key -> takes ownership
Review Comment:
`celix_stringHashMap_putXXX` no longer returns boolean value.
Documentation of `opts.removedKeyCallback` should also be updated.
##########
libs/utils/gtest/src/CxxPropertiesTestSuite.cc:
##########
@@ -52,14 +52,37 @@ TEST_F(CxxPropertiesTestSuite, testFillAndLoop) {
EXPECT_EQ(props.getAsBool("key5", false), true);
int count = 0;
+ for (auto it = props.begin(); it != props.end(); ++it) {
Review Comment:
Is this equivalent to the following range-based loop? Why shall we need both?
--
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
