Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add C++20 module build target v2 #1410

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Gaspard--
Copy link
Contributor

@Gaspard-- Gaspard-- commented Oct 22, 2024

This is a cleaner attempt at #1364 which hopefully is more maintainable and approchable.
While #1364 tried to tag each symbol, this PR tags namespaces instead, reducing the amount of defines that need to be inserted.

Usage in client cmake:

set(FLECS_M0DULE ON CACHE BOOL "Build flecs module" FORCE)
add_subdirectory(path/to/flecs)
target_compile_features(flecs-module PUBLIC cxx_std_23)
target_link_libraries(
   someProjectName
   PRIVATE
   flecs-module
)

In the code, it is then possible to use import flecs; to import all the flecs symbols. The current version includes everything in the flecs namespace, but avoids including anything in the _ namespace.

Maintainance:

  • namespaces that contain symbols to exported must be tagged with FLECS_API_NAMESPACE
  • namespaces that should be excluded should be seperate in order to avoid exporting symbols
  • static globals must be tagged with FLECS_STATIC_IN_HEADER, which becomes static in headers, but not in the module
  • some rare symbols outside namesapces are necessary for compilation, such as operator new / delete, and must be tagged FLECS_API_DEPENDENCY

I'd ideally like to find a way to modify the tests to import flecs instead of including it, but I don't know enough about bake to know if it's even possible (which seems unlikely if bake only works with includes).

@Gaspard-- Gaspard-- force-pushed the flecs_cpp20_module_v2_no_whitespace branch 3 times, most recently from 0bec791 to 9fe2bf1 Compare October 22, 2024 12:55
Usage in client cmake:
```cmake
set(FLECS_M0DULE ON CACHE BOOL "Build flecs module" FORCE)
add_subdirectory(path/to/flecs)
target_compile_features(flecs-module PUBLIC cxx_std_23)
target_link_libraries(
   someProjectName
   PRIVATE
   flecs-module
)
```

In the code, it is then possible to use `import flecs;` to import all the flecs symbols.
The current version includes everything in the `flecs` namespace, but avoids including anything in the `_` namespace.

Maintainance:
- namespaces that contain symbols to exported must be tagged with FLECS_API_NAMESPACE
- namespaces that should be excluded should be seperate in order to avoid exporting symbols
- static globals must be tagged with `FLECS_STATIC_IN_HEADER`, which becomes `static` in headers, but not in the module
- some rare symbols outside namesapces are necessary for compilation, such as operator new / delete, and must be tagged FLECS_API_DEPENDENCY
@Gaspard-- Gaspard-- force-pushed the flecs_cpp20_module_v2_no_whitespace branch from 9fe2bf1 to 8a02c98 Compare October 31, 2024 10:25
@SanderMertens
Copy link
Owner

This is a huge change which adds more complexity to the already non-trivial symbol exporting options of Flecs. When discussed on Discord you mentioned that besides being able to do

import flecs;

instead of

#include "flecs.h"

there aren't many benefits. Are there other benefits that are worth taking into account? Otherwise I'm not sure if the size/complexity of the change (and the maintenance effort going forward) balances out the pros.

@Gaspard--
Copy link
Contributor Author

There is a compilation speed benefit which is the main reason to do this.
I'm fine with this not being merged, I'll try to update the branch from time to time for anyone that might want this.
While the changes may seem big, they're pretty "simple", so it's not too hard.
I think it's likely that request for module support might increase as adoption becomes more common, so someone might pick this up in the future.
Feel free to close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants