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

C++20 module support #104

Draft
wants to merge 84 commits into
base: develop
Choose a base branch
from

Conversation

anarthal
Copy link

Adds support for import boost.mp11.
This pull request is intended to get feedback and support discussion in the ML. It depends on changes in Boost.CMake, Boost.Core, Boost.ThrowException and Boost.Assert that aren't in develop.
Any feedback is welcome.

@pdimov
Copy link
Member

pdimov commented Jan 15, 2025

Looks nicer.

Why do we have to include mp11.hpp in the GMF though? Can't we attach things to the named module?

@anarthal
Copy link
Author

For mp11 in concrete, yes, we can. It would require removing the cassert include and placing it in the GMF. It likely requires reverting to the BOOST_MP11_EXPORT macros we used to have.

For libraries that need to access entities in the detail namespace in tests (charconv being the one I've experimented with), this is not possible, since it causes redefinition errors for the tests that include detail headers.

Is there a benefit to having names attached to the named module? (Genuine question that I don't know the answer to).

@pdimov
Copy link
Member

pdimov commented Jan 16, 2025

I don't understand why we'd need <cassert> in the GMF, and I also don't understand why we'd need to revert to the export macros. Are we aiming to support configurations where BOOST_USE_MODULES is inconsistently defined in TUs? Should we?

IIUC the compiler needs to perform less work for names in the named module, because ODR is guaranteed and there's no need to merge declarations.

@anarthal
Copy link
Author

I don't understand why we'd need in the GMF

If we do:

export module boost.mp11;
#define BOOST_MP11_INTERFACE_UNIT
#include <boost/mp11.hpp> // Has an #include <cassert>

export namespace boost::mp11 { /* ... */ }

Any implementation-defined C++ entity declared in cassert get incorrectly attached to the boost.mp11. If we used the new boost/config/std/cassert.hpp header that just imports std, we don't get the assert macro.

Now, the above actually fails because apparently, in a module unit, imports are only allowed immediately after the module declaration:

[build] In file included from [redacted]/boost-with-modules/libs/mp11/modules/boost_mp11.cppm:9:
[build] In file included from [redacted]/boost-with-modules/libs/mp11/include/boost/mp11.hpp:21:
[build] In file included from [redacted]/boost-with-modules/libs/mp11/include/boost/mp11/tuple.hpp:24:
[build] [redacted]/boost-with-modules/libs/mp11/include/boost/mp11/detail/std/type_traits.hpp:2:1: error: imports must immediately follow the module declaration
[build]     2 | import std;
[build]       | ^
[build] In file included from [redacted]/boost-with-modules/libs/mp11/modules/boost_mp11.cppm:9:
[build] In file included from [redacted]/boost-with-modules/libs/mp11/include/boost/mp11.hpp:21:
[build] In file included from [redacted]/boost-with-modules/libs/mp11/include/boost/mp11/tuple.hpp:25:
[build] [redacted]/boost-with-modules/libs/mp11/include/boost/mp11/detail/std/cstddef.hpp:2:1: error: imports must immediately follow the module declaration
[build]     2 | import std;

We could work this around by making boost/config/std/$HEADER.hpp do nothing for module units (e.g. if BOOST_CONFIG_MODULE_UNIT is defined or something like this).

I also don't understand why we'd need to revert to the export macros.

I thought MSVC didn't like export using with names attached to a module. Let me double-check this.

Are we aiming to support configurations where BOOST_USE_MODULES is inconsistently defined in TUs? Should we?

No, and I don't think so.

@anarthal
Copy link
Author

I was right, entities attached to modules can't be exported with export using. This is true even in clang:

[build] [redacted]/boost-with-modules/libs/mp11/modules/boost_mp11.cppm:16:13: error: using declaration referring to 'mp_transform' with module linkage cannot be exported
[build]    16 | using mp11::mp_transform;

So if we get down this path, we need to revert to the BOOST_MP11_EXPORT macros.

@anarthal
Copy link
Author

What would you finally like to do with this? I see three options:

  1. Throw it away and forget about it
  2. Wait until modules are more mature - e.g. until MSVC fixes the two bugs I raised and CMake's import std becomes stable.
  3. Merge it for this or next release as experimental - I'd put some time in cleaning up CIs and writing docs.

IMO 2 is the more reasonable. 3's only point would be pushing modules forward.

@pdimov
Copy link
Member

pdimov commented Jan 24, 2025

(2), I think.

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