[GitHub] [celix] PengZheng commented on a diff in pull request #446: Feature/separate deprecated headers

[GitBox <gi...@apache.org>]

PengZheng commented on code in PR #446:
URL: https://github.com/apache/celix/pull/446#discussion_r1032878510


##########
cmake/celix_project/CelixProject.cmake:
##########
@@ -83,5 +83,31 @@ MACRO(celix_subproject)
     ENDIF (${NAME})
 ENDMACRO(celix_subproject)
 
+#[[
+Custom target which list the Celix CMake targets that are still using deprecated headers.
+]]
+if (NOT TARGET celix-deprecated)
+    add_custom_target(celix-deprecated
+        COMMAND ${CMAKE_COMMAND} -E echo "Targets still using deprecated utils headers: $<JOIN:$<TARGET_PROPERTY:celix-deprecated,UTIL_TARGETS>, >"
+    )
+    set_target_properties(celix-deprecated PROPERTIES "UTIL_TARGETS" "")
+endif ()
+
+
+#[[
+Add include path for the Celix utils deprecated headers to the provided target (as PRIVATE)
+
+```CMake
+celix_deprecated_utils_headers(<target_name>))
+```
+]]
+function(celix_deprecated_utils_headers)
+    list(GET ARGN 0 TARGET_NAME)
+    target_include_directories(${TARGET_NAME} PRIVATE ${CMAKE_SOURCE_DIR}/libs/utils/include_deprecated)
+    get_target_property(TARGETS celix-deprecated "UTIL_TARGETS")
+    list(APPEND TARGETS ${TARGET_NAME})
+    set_target_properties(celix-deprecated PROPERTIES "UTIL_TARGETS" "${TARGETS}")

Review Comment:
   ```suggestion
       set_properties(TARGET celix-deprecated APPEND PROPERTY "UTIL_TARGETS" "${TARGET_NAME}")
   ```
   
   This is slightly more compact.



##########
libs/utils/src/properties.c:
##########
@@ -467,11 +468,42 @@ int celix_properties_size(const celix_properties_t *properties) {
 }
 
 celix_properties_iterator_t celix_propertiesIterator_construct(const celix_properties_t *properties) {
-    return hashMapIterator_construct((hash_map_t*)properties);
+    celix_properties_iterator_t iter;
+    hash_map_iterator_t mapIter = hashMapIterator_construct((hash_map_t*)properties);
+    iter._data1 = mapIter.map;
+    iter._data2 = mapIter.next;
+    iter._data3 = mapIter.current;
+    iter._data4 = mapIter.expectedModCount;
+    iter._data5 = mapIter.index;

Review Comment:
   What about the following?
   
   ```C
   CELIX_BUILD_ASSERT(sizeof(celix_properties_iterator_t) == sizeof(hash_map_iterator_t));
   CELIX_BUILD_ASSERT(__alignof__(celix_properties_iterator_t) == __alignof__(hash_map_iterator_t));
   ```
   
   So that we can safely cast `celix_properties_t*` to ` hash_map_iterator_t*` to avoid any runtime overhead.



##########
libs/utils/include/celix_properties.h:
##########
@@ -18,10 +18,8 @@
  */
 
 #include <stdio.h>
-
-#include "hash_map.h"
-#include "exports.h"
 #include "celix_errno.h"
+#include "celixbool.h"

Review Comment:
   Now that we are using c99, we should use <stdbool.h> throughout? 



##########
CMakeLists.txt:
##########
@@ -170,24 +170,15 @@ if (ENABLE_TESTING)
 endif()
 
 option(CELIX_INSTALL_DEPRECATED_API "whether to install (and use) deprecated apis (i.e. header without a celix_ prefix." ON)
-
-option(CELIX_ADD_DEPRECATED_ATTRIBUTES "If enabled add deprecated attributes to deprecated services/functions." ON)

Review Comment:
   Generally, conan options can not be removed easily.
   But in this very special case, it's relatively safe to assume that no one touched `celix_add_deprecated_attributes` and `celix_add_openssl_dep`. Thus I suggest remove them altogether.



-- 
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-unsubscribe@celix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org