Better CMake config, standard #include orders#83
Open
jfsmig wants to merge 6 commits intoniXman:masterfrom
Open
Better CMake config, standard #include orders#83jfsmig wants to merge 6 commits intoniXman:masterfrom
jfsmig wants to merge 6 commits intoniXman:masterfrom
Conversation
There was some room for improvements, at least to ease using binapi as an ExternalProject
x-mass
reviewed
Nov 16, 2025
| ${BOOST_INCLUDE_DIR} | ||
| ) | ||
| endif() | ||
| target_compile_options(binapi PUBLIC |
There was a problem hiding this comment.
Suggested change
| target_compile_options(binapi PUBLIC | |
| target_compile_options(binapi PRIVATE |
You probably don’t want to propagate these options to users.
Also, -s is a linker option, I'm not sure it has any effect here. Better to move to target_link_options.
x-mass
reviewed
Nov 16, 2025
Comment on lines
+42
to
+55
| add_executable(example_asynchronous-user_data examples/asynchronous-user_data/main.cpp) | ||
| target_link_libraries(example_asynchronous-user_data binapi) | ||
|
|
||
| add_executable(example_synchronous examples/synchronous/main.cpp) | ||
| target_link_libraries(example_synchronous binapi) | ||
|
|
||
| add_executable(example_synchronous-user_data examples/synchronous-user_data/main.cpp) | ||
| target_link_libraries(example_synchronous-user_data binapi) | ||
|
|
||
| add_executable(example_websockets examples/websockets/main.cpp) | ||
| target_link_libraries(example_websockets binapi) | ||
|
|
||
| add_executable(example_api main.cpp) | ||
| target_link_libraries(example_api binapi) |
There was a problem hiding this comment.
add_subdirectory or FetchContent_Declare will lead to building of all these targets. Better to either add an option to conditionally build them or set EXCLUDE_FROM_ALL for each.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The present CMake configuration helps importing binapi as an ExternalProject in an upper layer.
The order of includes has been reviewed to respect the generally-admitted standard of "C system, C++ system, C deps, C++ deps, local".
Also, CMake packages are used to locate the dependencies.