Conversation
…ke into feature/cxx20-modules
|
Rendered document talking about the modularization strategy: https://github.com/anarthal/boost-cmake/blob/feature/cxx20-modules/modules.md |
|
@DanielaE @ClausKlein this is similar to what you've been doing in https://github.com/ClausKlein/asio/ but for the rest of Boost. It'd be helpful if you could have a look to the proposed strategy and some of the PRs and provide some feedback. Thanks. |
|
@ChuanqiXu9 FYI, your feedback also welcome |
|
I don't think I like putting the stdlib forwarding headers in Config. I'd rather include Related, at the moment we have a single macro for "use import std" and "create boost.foo modules". I think that using Why do we need the "in module purview" check? |
I can create a separate repo.
Because imports in a module must follow the module unit declaration. This is illegal: module x;
import a;
int i = 42;
import b; // should be before the int declThis constraint does not apply to non-module code. See this for the rationale: https://github.com/anarthal/boost-cmake/blob/feature/cxx20-modules/modules.md#creating-the-compatibility-headers
This has edges. Something I could potentially do is:
I can also do the other way around (i.e. Note however that this errors on I'd say the 3 compilers: import std;
#include <cmath>Although it should be legal, but things are what they are. |
|
There's not much gain from not including But this same reasoning also applies to the feature macros. It seems to me that rather than just |
|
Is it not? Many functions there are supposed to be constexpr so I assume there's some non-trivial things going on in this header. I haven't measured though. I agree that this is a special case though. cerrno, cfloat, cassert and friends are probably trivial enough. Not sure about cstdio though. The version addition makes sense. |
ChuanqiXu9
left a comment
There was a problem hiding this comment.
Excellent doc!
A few points:
For converting the library, I suggest to define a macro:
#ifdef BOOST_IN_MODULE_PURVIEW
#define BOOST_INLINE
#else
#define BOOST_INLINE inline
#endif
Then we can put this in front of boost's inline functions.
The less inline code in the module purview, the compile time improvements and code size improvements are bigger for users.
Although I already see you mentioned you worry about distribution problems, but given you have to have an object binary in the end, I feel it might be solvable problem. Especially they are users who build every thing from source.
As a user, I feel better to see a section how can we workaround when the official support is not landed. e.g, in this doc, we can say the users can wrap boost module itself if they want to use it. For example, I have: https://github.com/ChuanqiXu9/beast and it seems like there are already some use examples: https://github.com/search?q=%22import+boost%3B%22+language%3AC%2B%2B&type=code . And in this doc, we can say, boost didn't choose the simple way to wrap boost module since boost has a higher bar to ship features.
|
|
||
| // Including headers with #include <> in the purview triggers warnings. | ||
| // If you're following this guide, they should be safe to ignore. | ||
| // This header disables them |
There was a problem hiding this comment.
Maybe we can use #include "header.h"?
There was a problem hiding this comment.
The rule applies recursively. Even if you use #include "boost/xyz.hpp", you will get warnings for all the other headers included by boost/xyz.hpp, which use angle-brackets.
There was a problem hiding this comment.
BTW, I've found what I think is a problem in clang-scan-deps - while clang respects my warning suppression, clang-scan-deps does not and emits a tons of warnings during scanning. Do you want me to provide a minimal reproducible bug report for this? Or is it a known issue?
There was a problem hiding this comment.
I didn't heard it. I think a bug report may be worthy.
There was a problem hiding this comment.
| Some notes: | ||
|
|
||
| * We use `STATIC` (rather than letting the user choose) because the | ||
| binary is expected to contain almost no code. This simplifies |
There was a problem hiding this comment.
binary is expected to contain almost no code
This may not be true with ABI Breaking style as you put headers in the module purview, the in-class defined functions are not inlined right now. Maybe you can double check this.
There was a problem hiding this comment.
I recommend in a later paragraph to stick an explicit inline in front of member functions.
I think allowing SHARED here is opening a can of worms. You now need to start worrying about __declspec(dllexport) in Windows and symbol visibility under Linux. Also, I don't think that simple getters should end up behind a DLL boundary. At the very minimum, authors need to be aware of this and choose to enable this explicitly.
There was a problem hiding this comment.
Understood. As I am not boost's dev, I won't push on it. Just my 2 cents. My opinion is, generally we don't have to put an inline in front of member functions. And as doc, i feel it is better to say: if you want to provide it as shared, be careful about the symbol exported in shared libraries.
There was a problem hiding this comment.
I'll add a comment explaining the tradeoffs then. I think it may be worth not inlining some of the functions. Author's choice seems fine to me.
|
|
||
| Some notes: | ||
|
|
||
| * We use `STATIC` (rather than letting the user choose) because the |
There was a problem hiding this comment.
I feel shared library may be fine too. Is it a problem for you?
There was a problem hiding this comment.
See the above point.
| Member functions in non-module code are implicitly inline. | ||
| This is no longer true in module code. If you want functions to remain | ||
| inline, you need to mark them explicitly: |
There was a problem hiding this comment.
I think in most cases we don't have to do so?
There was a problem hiding this comment.
For correctness, no. For performance, I think this is still to be determined, some compilers will factor this into their decisions on inlining. I'm fairly sure that I already experienced one case where I lost some significant performance because an extremely trivial function ended up not being inlined. Of course it depends on many factors including LTCG, but anyway I think it's good to be aware of the difference.
There was a problem hiding this comment.
For correctness, no. For performance, I think this is still to be determined, some compilers will factor this into their decisions on inlining. I'm fairly sure that I already experienced one case where I lost some significant performance because an extremely trivial function ended up not being inlined. Of course it depends on many factors including LTCG, but anyway I think it's good to be aware of the difference.
Yeah, it is better to mention it explicitly. For runtime performance, my experience is, we should use LTO/ThinLTO with modules to get best performance. I don't feel this is extra requirement to me. Since previously we already need LTO/ThinLTO to get best performance.
Does your module implementation work well with clang-22, which enables global module fragment discards by default? I've done an experiment in a pet project which uses Beast (anarthal/servertech-chat#77) and the results were frankly bad. Exporting My takeaway from this experiment is "export using does not work in the general case". It might work for std or for libraries only exporting functions, but it seems to fail for heavily templated code. So I'm not very keen on recommending it at this point, unless I'm missing something. |
I am using latest trunk. I didn't met your problem. For coroutines, I use my library: https://github.com/alibaba/async_simple. And I didn't find problems yet. For the thread itself, I think you made the right choice. Although it requires harder work, the ABI breaking style may be the best in the long term.
I didn't met this. Techniquelly, the compiler is allowed to elide things which it think unnecessary ("unnecessary" is almost implementation defined here.) And if we want something to keep unelided, we can using it without exporting it. The exporting using style have problems, e.g, it can't using friend entities. And most existing users of C++20 modules use |
| a typical Boost `CMakeLists.txt`: | ||
|
|
||
| ```cmake | ||
| cmake_minimum_required(VERSION 3.5...3.31) |
There was a problem hiding this comment.
The import std; is starts really to be usable with cmake v4.2, better witch current v4.3-rec2.
But it seems still experimanteal at the moment!
There was a problem hiding this comment.
Yep, I think we will merge this once CMake makes it stable (4.4 I guess?). The minimum can't be bumped to that high because we intend to keep supporting headers and older systems. I should update the higher version range though.
| else() | ||
| add_library(boost_xyz INTERFACE) | ||
| set(__scope INTERFACE) | ||
| endif() |
There was a problem hiding this comment.
Why not always add an header only interface lib too where it is possible?
This would allow to use a FILE_SET HEADERS and enable VERIFY_INTERFACE_HEADER_SETS.
There was a problem hiding this comment.
How would that this interface library be used? If building with BOOST_USE_MODULES, headers expand to imports so using it directly would lead to missing modules.
There was a problem hiding this comment.
I dit it on my asio feature/module branch.
The problem would be the installation: you need to install both libraries, and one is optional!
I have written a generic install cmake module for the Beman project to handle this.
| Partition units need to be compiled. Since they can be imported, they | ||
| must be in a CMake `CXX_MODULES FILE_SET`. Because they are internal | ||
| and shouldn't be installed, they should be `PRIVATE`: | ||
|
|
||
| ```cmake | ||
| target_sources(boost_xyz | ||
| PUBLIC FILE_SET CXX_MODULES BASE_DIRS modules FILES | ||
| modules/boost_xyz.cppm | ||
| PRIVATE FILE_SET source_headers TYPE CXX_MODULES BASE_DIRS src FILES | ||
| src/base64.cppm | ||
| ) | ||
| ``` |
There was a problem hiding this comment.
Dit you test this?
My experience is: if an exported cmake config package with modules is used by an consumer,
CMake build the CXX_MODULE again. All source seems needed to be installed!
| ### Source-only headers | ||
|
|
||
| Functionality shared by several `.cpp` files is usually placed into | ||
| header files that live within `src/`. These headers are "source-only": | ||
| they don't get installed like the ones under `include/`. |
There was a problem hiding this comment.
Dit you test this?
My experience is: if an exported cmake config package with modules is used by an consumer,
CMake build the CXX_MODULE again. All source seems needed to be installed!
There was a problem hiding this comment.
I think I did. But I will double-check today. My impression is that only public file sets get installed and are needed to build BMIs at the consumer side. This would be consistent with having partition implementation units (i.e. partitions that are not exported), which don't contribute to the module interface. See boostorg/charconv#292 for how I intend to use this. All Boost libraries contain tests to check that what we export is usable, so this will be caught in case it would create problems.
There was a problem hiding this comment.
I had the same problem with your module-playground project. PRIVATE header are never installed but needed for module consumers!
There was a problem hiding this comment.
All Boost libraries contain tests to check that what we export is usable, so this will be caught in case it would create problems.
maybe, but on your charconv branch, the roundtrip test does not use the cxx_modules variant for test.
There was a problem hiding this comment.
Does the posix-cmake-install-modules GHA flow not test exactly this? Am I doing something stupid?
There was a problem hiding this comment.
check for target_include_directories(boost_charconv PUBLIC include)
That is wrong, you must use file_set headers, to be clean.
There was a problem hiding this comment.
I tested with cmake_install_test/CMakeLists.txt:
cmake_minimum_required(VERSION 3.21...4.3)
project(cmake_install_test LANGUAGES CXX)
find_package(boost_charconv 1.90.0 EXACT REQUIRED)
add_executable(quick ../quick.cpp)
target_link_libraries(quick PRIVATE Boost::charconv)
if(BOOST_USE_MODULES)
# Enable and propagate C++23, import std, and the modules macro
target_compile_features(quick PUBLIC cxx_std_23)
set_target_properties(quick PROPERTIES CXX_MODULE_STD 1)
endif()
enable_testing()
add_test(quick quick)
add_custom_target(check COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure -C $<CONFIG>)and quick.cpp:
// Copyright 2022 Peter Dimov
// Distributed under the Boost Software License, Version 1.0.
// https://www.boost.org/LICENSE_1_0.txt
#if defined(__GNUC__) && __GNUC__ == 11
# pragma GCC diagnostic push
# pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
#endif
#ifdef BOOST_USE_MODULES
import boost.charconv;
#else
#include <boost/charconv.hpp>
#endif
int main()
{
char buffer[ 32 ];
auto r = boost::charconv::to_chars( buffer, buffer + sizeof( buffer ), 1048576 );
int v = 0;
boost::charconv::from_chars( buffer, r.ptr, v );
}There was a problem hiding this comment.
I see that IMPORTED_CXX_MODULES_INCLUDE_DIRECTORIES there is wrong. So this is not about headers under src/, but public headers (the ones under include/). I don't think I can just always use a FILE_SET because of backwards compatibility requirements. Boost.CMake handles this by directly updating INTERFACE_INCLUDE_DIRECTORIES and friends (see
cmake/include/BoostInstall.cmake
Line 281 in 7bce8ef
If I install(DIRECTORY include) and also install the headers file set, am I expected to find problems? (I can't get rid of the install(DIRECTORY include)).
There was a problem hiding this comment.
Does the
posix-cmake-install-modulesGHA flow not test exactly this? Am I doing something stupid?
I do NOT know this? neither GHA nor posix-cmake-install-modules
There was a problem hiding this comment.
If I
install(DIRECTORY include)and also install the headers file set, am I expected to find problems? (I can't get rid of theinstall(DIRECTORY include)).
I know, and it is not wrong, with boost install!
But I tested on my branch with boots::any and use FILE_SET HEADERS too.
This breaks the backward compatibility, but I want to be clean and accurate!
|
@anarthal please note my PR Modernise cmake and add a round trip test |
Adds support to install CXX_MODULES FILE_SETs
Adds a document with the recommended modularization stratey