Skip to content

Conversation

@bangerth
Copy link
Contributor

@bangerth bangerth commented May 9, 2025

I am working on converting a large scientific library (https://www.dealii.org) that builds on BOOST to C++20 modules, see dealii/dealii#18071. As part of this effort, I have to find ways to "wrap" BOOST into a module partition, something that will look like this file: https://github.com/bangerth/dealii/blob/cxx-modules/cxx20-modules/interface_module_partitions/boost.ccm

Wrapping works by "exporting" certain BOOST names via using declarations, but the targets of these using declarations can only be names that have "external linkage". This excludes static variables and functions because they have "internal linkage". GCC 15 also requires that all names referenced by entities I want to export also have external linkage, and this turns up quite a large number of static variables and functions in BOOST header files.

There are good reasons to avoid internal linkage variables and functions in header files beyond my desire to use C++20 modules. In particular, they should be avoided because they are replicated in each translation unit that #includes a header file; if the object had external linkage, the linker would unify these copies into one, reducing code size.

This patch addresses a number of places where this BOOST sub-library uses names with internal linkage in header files, by removing the static keyword and, where applicable, marking variables as inline. (Variables marked as const or constexpr implicitly have internal linkage unless marked with inline.)

@barendgehrels
Copy link
Collaborator

Thanks. I have no objections (apart from one question), it's just that there are much more functions that are marked as static inline and they are not handled. But we can do that later.

@bangerth
Copy link
Contributor Author

bangerth commented May 9, 2025

Yes, there are hundreds of such functions throughout BOOST :-) I'm an incrementalist in that I think that starting somewhere is better than waiting for a patch that fixes everything. So this is an increment :-)

Would you like me to update the patch to use constexpr instead of const? Separately, inline variables are a C++17 thing. Does BOOST require C++17 these days?

@barendgehrels
Copy link
Collaborator

Separately, inline variables are a C++17 thing. Does BOOST require C++17 these days?

It depends per library. Boost.Geometry is at C++14 so the inline should be removed for the variable. But you can make it constexpr, please do. Hope that will be sufficient for your use case.

@barendgehrels
Copy link
Collaborator

I'm an incrementalist in that I think that starting somewhere is better than waiting for a patch that fixes everything.

I see, thanks. It just seemed a bit a random selection. I think we can remove all of them in one go (maybe with a few exceptions)

@bangerth
Copy link
Contributor Author

bangerth commented May 9, 2025 via email

@barendgehrels
Copy link
Collaborator

barendgehrels commented May 10, 2025

To export the variable in a C++20 module, I need it to have external linkage, which I can only get by using 'inline'. Is there a BOOST macro that makes something inline if we use C++17 or later, and expands to nothing for C++14 and earlier?

I see, thanks.

Yes, there is, you can use BOOST_INLINE_CONSTEXPR as done here, this is probably the most convenient because it does both things (inline and constexpr)

Alternatively there is also BOOST_INLINE_VARIABLE

@barendgehrels
Copy link
Collaborator

It's included by #include <boost/config.hpp> (but it's implicitly included already by boost/geometry/config.hpp)

@barendgehrels
Copy link
Collaborator

barendgehrels commented May 10, 2025

About my remark

it's just that there are much more functions that are marked as static inline and they are not handled.

I think you have most, or all, already in Boost.Geometry. Thanks!

So after changing inline to BOOST_INLINE_CONSTEXPR as discussed we seem to be good to go and I'll approve.

Copy link
Collaborator

@tinko92 tinko92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, if this needs inline I'd certainly be in favour of the constexpr inline macro to make the constexpr explicit, even when it is implied.

Is this change made redundant by DR2387 applied to C++14 and is there a compiler with sufficiently working module support to work for boost but no implementation of DR2387? If the answer is no to the first or yes to the second then I'm in favour of merging with use of the macro.

Copy link
Collaborator

@barendgehrels barendgehrels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@bangerth
Copy link
Contributor Author

I marked the two variables as BOOST_INLINE_CONSTEXPR. Let's see what your CI says.

@bangerth
Copy link
Contributor Author

@tinko92:

Is this change made redundant by DR2387 applied to C++14 and is there a compiler with sufficiently working module support to work for boost but no implementation of DR2387? If the answer is no to the first or yes to the second then I'm in favour of merging with use of the macro.

DR2387 is unrelated. As for the compiler I'm using: Clang++-20 is sufficient for me to build modules for a project with 1000+ files and 1M+ lines of code. In fact, Clang-20 does not complain about the code I'm fixing here (with the exception of the two variables that need to be inline). It is GCC-15 that is more picky and complained about the places I'm pointing out here. GCC-15 has many other problems I am still working around.

@barendgehrels
Copy link
Collaborator

@bangerth it's all green. I'll merge it right now. Thanks again for the contribution.

@barendgehrels barendgehrels merged commit fd6d0e7 into boostorg:develop May 15, 2025
18 checks passed
@bangerth bangerth deleted the static branch May 15, 2025 18:45
@bangerth
Copy link
Contributor Author

Thank you, too! I will likely have more patches of this kind to various BOOST libraries.

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.

4 participants