cleanup: don't use active_class_loaders_[library_path] for existence test #35
Conversation
rhaschke
commented
Apr 3, 2016
rhaschke
commented
- use find instead of active_class_loaders_[library_path] for existence test
- the latter was accumulating map entries with NULL pointers
- fixing it, allows some cleanup
…test - this accumulates map entries with NULL pointer - fixing it, allows some cleanup
|
looks good to me. |
| @@ -37,7 +37,8 @@ set(${PROJECT_NAME}_SRCS | |||
| src/meta_object.cpp | |||
| src/multi_library_class_loader.cpp | |||
| ) | |||
| add_library(${PROJECT_NAME} ${${PROJECT_NAME}_SRCS}) | |||
| file(GLOB ${PROJECT_NAME}_HDRS "include/${PROJECT_NAME}/*.h") # allow headers to be shown in CodeBlocks/QtCreator | |||
There was a problem hiding this comment.
Globbing source files in CMake is discouraged: https://cmake.org/cmake/help/v3.5/command/file.html?highlight=glob#file
We do not recommend using GLOB to collect a list of source files from your source tree. If no CMakeLists.txt file changes when a source is added or removed then the generated build system cannot know when to ask CMake to regenerate.
There was a problem hiding this comment.
I know. However, finding the headers is not functionally relevant here (dependency analysis will be performed by make).
It only helps some IDEs like CodeBlocks and QtCreator to list all headers of the project too. And for that purpose globbing is perfect. One doesn't need to care about it anymore ;-)
There was a problem hiding this comment.
I'd advocate not to do it here given that no other package in the codebase aggregates headers globally for IDE purposes.
There was a problem hiding this comment.
OK. I will list all headers explicitly then.
|
@ros-pull-request-builder could you retest this please? |
