modules: make module support standards-compliant #3101
Conversation
C++ doesn't allow #ifdef tricks with modules. Specifically, the module keywords can't be guarded with #ifdef. Here, we change the approach towards module support. It is heavily inspired by libstdc++ [1]. In the new approach, instead of two libraries, seastar and seastar-module, one without and one with module support, there is only one library. Module support comes in the form of a .cppm file that declares the module and exports the symbols. Changes in the patch: 1. remove SEASTAR_MODULE_EXPORT* defines; the export declarations are moved to seastar.cppm (causing some duplication) 2. remove the SEASTAR_MODULE define and unifdef the code it guards. There is a single complication pass rather than two. 3. The utils/modules.hh header is removed, since it no longer defines anything. 4. seastar.cc is renamed to seastar.cppm so the compiler understands it's a module definition. It now only includes headers that contain public symbols. The symbols to be exported are listed explicitly. 5. The cmake seastar library now conditionally includes seastar.cppm. 6. Minor cmake changes for hello-cxx-module to make it build. [1] https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/src/c%2B%2B23/std.cc.in Tested with clang 21. Fixes scylladb#3091
|
The motivation here is not so much to fix modules (the ecosystem is still not there), but to allow us to bump the compiler versions. |
|
/cc @tchaikov |
| using seastar::http::reply; | ||
| using seastar::http::request; | ||
|
|
||
| } |
There was a problem hiding this comment.
The list of exports from seastar::http looks too small. Why aren't e.g. http_server_control and experimenta::client exported?
There was a problem hiding this comment.
This part was done by copilot and I didn't examine it too closely. We can build it on demand as we discover missing items.
I predict demand will be zero until ccache/distcc support [1] modules. Without that, it's not possible to integrate modules into a large system.
btw, I tried to have copilot prepare [1] this entire pull request, but it didn't work well. In the end I used sed and unifdef, and asked copilot to help with cmake and this last patch.
[1] ccache/ccache#1523
[2] avikivity#1
|
Patch comment:
|
| using seastar::metrics::label_instance; | ||
|
|
||
| // Utility functions | ||
| using seastar::do_with; |
There was a problem hiding this comment.
The do_with() is going to be deprecated in favor of using coroutines. Maybe not export it with modes right at once?
There was a problem hiding this comment.
I don't think it's modules' job to control deprecation, it's just an alternative to headers.
There was a problem hiding this comment.
I didn't mean to deprecate it here, but rather not to export the thing that's going to be deprecated later anyway. But taking into account this comment, I no longer have this concern
There was a problem hiding this comment.
I also don't think that modules has anything to do with whether we deprecate do_with() or not.
I think that as long we we don't forbid the old continuation style, I don't think we should remove or even deprecate the old idioms that were useful for code written in continuation style. Otherwise, imagine someone will be writing in Seastar with modules and needs (for some reason or another, perhaps a small performance advantage or copy-pasting old code) to use continuation style. Why shouldn't they be able to use do_with()?
There was a problem hiding this comment.
Right, and we have some do_with code like that.
if (fast_path) {
return make_ready_future...
} else {
return do_with ...
}A workaround is to split the slow path into a function and make that a coroutine. But I see no reason to force that.
|
v2: fixed spelling error in patch changelog (though the original spelling was oddly suitable) |
C++ doesn't allow #ifdef tricks with modules. Specifically, the module keywords can't be guarded with #ifdef. Here, we change the approach towards module support. It is heavily inspired by libstdc++ [1]. In the new approach, instead of two libraries, seastar and seastar-module, one without and one with module support, there is only one library. Module support comes in the form of a .cppm file that declares the module and exports the symbols. Changes in the patch: 1. remove SEASTAR_MODULE_EXPORT* defines; the export declarations are moved to seastar.cppm (causing some duplication) 2. remove the SEASTAR_MODULE define and unifdef the code it guards. There is a single complication pass rather than two. 3. The utils/modules.hh header is removed, since it no longer defines anything. 4. seastar.cc is renamed to seastar.cppm so the compiler understands it's a module definition. It now only includes headers that contain public symbols. The symbols to be exported are listed explicitly. 5. The cmake seastar library now conditionally includes seastar.cppm. 6. Minor cmake changes for hello-cxx-module to make it build. [1] https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/src/c%2B%2B23/std.cc.in Tested with clang 21. Fixes scylladb#3091 Closes scylladb#3101
3 participants
C++ doesn't allow #ifdef tricks with modules. Specifically, the module keywords can't be guarded with #ifdef.
Here, we change the approach towards module support. It is heavily inspired by libstdc++ [1]. In the new approach, instead of two libraries, seastar and seastar-module, one without and one with module support, there is only one library. Module support comes in the form of a .cppm file that declares the module and exports the symbols.
Changes in the patch:
[1] https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/src/c%2B%2B23/std.cc.in
Tested with clang 21.
Fixes #3091