PengZheng commented on code in PR #665:
URL: https://github.com/apache/celix/pull/665#discussion_r1348225256
##########
conanfile.py:
##########
@@ -47,8 +47,6 @@ class CelixConan(ConanFile):
"enable_address_sanitizer": False,
"enable_undefined_sanitizer": False,
"enable_thread_sanitizer": False,
- "enable_testing_dependency_manager_for_cxx11": False,
Review Comment:
The `description` above should also be updated.
##########
libs/framework/gtest/CMakeLists.txt:
##########
@@ -15,8 +15,6 @@
# specific language governing permissions and limitations
# under the License.
-set(CMAKE_CXX_STANDARD 17)
Review Comment:
But in `framework/CMakeLists.txt`:
```CMake
if (ENABLE_TESTING AND CELIX_CXX17) #framework tests are C++17
add_library(framework_cut STATIC ${FRAMEWORK_SRC})
```
If `framework/gtest` is now C++14, then `framework/CMakeLists.txt` should be
updated.
##########
libs/utils/gtest/CMakeLists.txt:
##########
@@ -38,10 +31,16 @@ add_executable(test_utils
src/ThreadsTestSuite.cc
src/CelixErrnoTestSuite.cc
src/CelixUtilsAutoCleanupTestSuite.cc
- ${CELIX_UTIL_TEST_SOURCES_FOR_CXX_HEADERS}
)
-target_link_libraries(test_utils PRIVATE utils_cut Celix::utils GTest::gtest
GTest::gtest_main libzip::zip)
+add_library(test_utils_cxx17tests STATIC
+ src/ArrayListTestSuite.cc #Uses constexpr
+ src/HashMapTestSuite.cc #Uses constexpr
+)
+target_link_libraries(test_utils_cxx17tests PRIVATE utils_cut Celix::utils
GTest::gtest GTest::gtest_main)
+target_compile_features(test_utils_cxx17tests PRIVATE cxx_std_17)
Review Comment:
Should it be protected by `if (CELIX_CXX17)`?
##########
libs/utils/gtest/CMakeLists.txt:
##########
@@ -38,10 +31,16 @@ add_executable(test_utils
src/ThreadsTestSuite.cc
src/CelixErrnoTestSuite.cc
src/CelixUtilsAutoCleanupTestSuite.cc
- ${CELIX_UTIL_TEST_SOURCES_FOR_CXX_HEADERS}
)
-target_link_libraries(test_utils PRIVATE utils_cut Celix::utils GTest::gtest
GTest::gtest_main libzip::zip)
+add_library(test_utils_cxx17tests STATIC
+ src/ArrayListTestSuite.cc #Uses constexpr
+ src/HashMapTestSuite.cc #Uses constexpr
+)
+target_link_libraries(test_utils_cxx17tests PRIVATE utils_cut Celix::utils
GTest::gtest GTest::gtest_main)
Review Comment:
Is `gtest_main` unnecessary for a static library?
##########
libs/utils/gtest/CMakeLists.txt:
##########
@@ -15,21 +15,14 @@
# specific language governing permissions and limitations
# under the License.
-set(CMAKE_CXX_STANDARD 17)
Review Comment:
Does this mean that `test_utils` is now C++14? If so, `utils/CMakeLists.txt`
should also be updated:
```CMake
if (CELIX_CXX17) #utils tests are C++17
add_subdirectory(gtest)
endif()
```
--
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
