PengZheng commented on code in PR #446:
URL: https://github.com/apache/celix/pull/446#discussion_r1051302703
##########
CMakeLists.txt:
##########
@@ -92,7 +92,7 @@ if (NOT DEFINED CMAKE_CXX_STANDARD)
endif ()
set(CMAKE_CXX_FLAGS "-fno-rtti ${CMAKE_CXX_FLAGS}")
-set(CMAKE_CXX_FLAGS "-Wall -Werror -Wextra -Weffc++ ${CMAKE_CXX_FLAGS}")
+set(CMAKE_CXX_FLAGS "-Wall -Werror -Wextra -Weffc++ -Werror
-Wno-error=deprecated-declarations ${CMAKE_CXX_FLAGS}")
Review Comment:
```suggestion
set(CMAKE_CXX_FLAGS "-Wall -Werror -Wextra -Weffc++
-Wno-error=deprecated-declarations ${CMAKE_CXX_FLAGS}")
```
Fix duplicated `-Werror`
##########
libs/utils/src/properties.c:
##########
@@ -21,12 +21,14 @@
#include <string.h>
#include <stdlib.h>
#include <ctype.h>
-#include "celixbool.h"
+#include <stdbool.h>
+
#include "properties.h"
Review Comment:
```suggestion
#include "properties.h"
#include "celix_build_assert.h"
```
##########
libs/utils/include_deprecated/utils.h:
##########
@@ -32,22 +32,24 @@
extern "C" {
#endif
-UTILS_EXPORT unsigned int utils_stringHash(const void *string);
+#define DEPRECATED_ATTR __attribute__((deprecated("functions in utils.h are
deprecated use functions in celix_utils.h instead")))
Review Comment:
`DEPRECATED_ATTR` is too common a name to use for a specific module. In
other word, it's easy to have a conflict with other library, even if there is
no conflict in Celix.
##########
bundles/shell/shell/gtest/src/ShellTestSuite.cc:
##########
@@ -33,13 +35,13 @@ class ShellTestSuite : public ::testing::Test {
}
static std::shared_ptr<celix_bundle_context_t> createFrameworkContext() {
- auto properties = properties_create();
- properties_set(properties, "LOGHELPER_ENABLE_STDOUT_FALLBACK", "true");
- properties_set(properties, "org.osgi.framework.storage",
".cacheShellTestSuite");
- properties_set(properties, "CELIX_LOGGING_DEFAULT_ACTIVE_LOG_LEVEL",
"trace");
+ auto properties = celix_properties_create();
+ celix_properties_set(properties, "LOGHELPER_ENABLE_STDOUT_FALLBACK",
"true");
Review Comment:
```suggestion
celix_properties_setBool(properties,
"LOGHELPER_ENABLE_STDOUT_FALLBACK", true);
```
##########
libs/utils/src/properties.c:
##########
@@ -467,11 +469,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;
Review Comment:
```suggestion
celix_properties_iterator_t iter;
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));
```
I'm OK with the current explicit way, but strongly recommend adding
build-time assertions in case of any future inconsistency.
--
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
