Initial support for the C++20 modules #191
Conversation
| @@ -22,6 +24,8 @@ namespace core | |||
| namespace detail | |||
| { | |||
|
|
|||
| BOOST_CORE_BEGIN_MODULE_EXPORT | |||
There was a problem hiding this comment.
I don't think this should be exported. core::detail::is_same is an implementation detail and should not be used by users.
There was a problem hiding this comment.
I also do not think that this should be exported.
However clang complains on exporting namespce core { using boost::core::detail::is_same; } without exporting this detail, so there's actually no choice if boost::core::is_same is expprted
There was a problem hiding this comment.
That using-declaration is for backward compatibility and deprecated, it should not be exported either but rather removed.
| #include <typeinfo> | ||
| #include <type_traits> | ||
| #include <utility> | ||
| #include <wchar.h> |
There was a problem hiding this comment.
Why are all these includes needed? Everything needed by Boost.Core should be included by Boost.Core headers.
There was a problem hiding this comment.
That is true for headers. But when a module is created, we need to place all the standard library and OS stuff into a global module fragment to avoid same symbols being ambiguously exported from different modules of Boost.
To do so I put all the headers right after the module;, so they end up in global module fragment. Later, after the export module Boost.Core; I include all the library heders. Include guards of the standard library and OS stuff do they work, so the library headers do not actually include them again. As a result, only the library classes, functions and aliases apear after export module Boost.Core; , so the module exports only the library stuff
There was a problem hiding this comment.
If supporting modules requires duplicating includes from all headers then the whole modules undertaking is a non-starter. This is unmaintainable. If we're to support modules, this should not be required.
There was a problem hiding this comment.
Now I also have a feeling that I'm doing something wrong. I'll try another approach soon
| #include <boost/noncopyable.hpp> | ||
| } | ||
| #else | ||
| #include <boost/visit_each.hpp> |
There was a problem hiding this comment.
Why the duplicate list of includes?
|
|
||
| export module Boost.Core; | ||
|
|
||
| #ifdef BOOST_CORE_HAS_STD_MODULE |
There was a problem hiding this comment.
This macro should be changed to some Boost.Config macro that is defined if the compiler understands modules and there is module std. The includes in the headers should be guarded by that macro and use import std; if it is available
|
? |
|
Oh, I misread this. @apolukhin you might have a look at the articles pointed out by Peter and the discussion we had on the ML. I will have a look at the PR on Monday. |
|
@anarthal yes, I saw the links before and followed the approach from the links. @Lastique noted that there's something wrong with such approach. Including all the standard headers before the I'm planing to do thefollowin experiment with module partitions: In each header a new module partition is declared and in PMI all of them are exported |
You also need to read the third post, which describes the approach we ultimately settled on. |
|
If we want to support dual builds, I strongly recommend being able to run the library's test suite under modular builds. Otherwise, we're just shipping untested code, which I don't think we should do. See how the two PRs mentioned in the articles have full test suites and CI builds: |
Oops, I was sure that saw it but now I realize the missed it. Thanks! @pdimov @anarthal I'll concentrate on C++20 modularization of libraries I maintain. Please, confirm that I understood the core principles correctly:
|
|
I'd say so, yes. Also, remember to add the relevant CMake and CI code. I'm happy to review the PRs for your libraries if it helps. |
The PR follows an experimental modules approach from Boost.PFR boostorg/pfr@6039165
The approach is an experiment. Any comments, suggestions are welcome!