boostorg / math

Boost.org math module
http://boost.org/libs/math
Boost Software License 1.0
309 stars 221 forks source link

Configurable top-level namespace (for standalone) #769

Open CaseyCarter opened 2 years ago

CaseyCarter commented 2 years ago

As you probably already know, MSVC uses Boost Math to implement the C++17 mathematical special functions. (My gratitude and joy over this fact cannot be expressed with words - I did not want to spend a year of my life becoming expert enough in numerical analysis to implement these properly myself!) We have a thin wrapper layer that adapts the Standard's API to Boost's, which works great when folks link to the STL dynamically. We do have an issue, however, in that symbols in namespace ::boost are directly visible in our static libraries, causing ODR problems for folks who are using different versions of Boost Math than we ship.

Would you be amenable to a PR that made the top-level namespace configurable if we were to implement such a feature? We could maintain a fork, of course, but (1) it seems like this might be useful for other folks that want to ship third-party libs using Boost Math, and (2) we like to keep the STL's list of dependencies short and clean to make it easier to onboard new contributors.

NAThompson commented 2 years ago

@jzmaddock will need to sign off, but a priori I see no reason why this shouldn't be done.

Have you tried the standalone? Might help to scope the problem a bit and I believe there is CMake tooling to help with this particular problem.

StephanTLavavej commented 2 years ago

Have you tried the standalone?

Yep! We've been using it since we merged https://github.com/microsoft/STL/pull/2151 in Aug 2021.

NAThompson commented 2 years ago

Could this be done via set_target_properties(boost_math PROPERTIES CXX_VISIBILITY_PRESET hidden)? You'll have to excuse my naivety if this is totally underestimates the effort; I could never figure out what symbol visibility is . . .

StephanTLavavej commented 2 years ago

According to my extremely vague understanding, that's already the default for MSVC, and it applies to DLLs. On the MSVC platform, we don't usually have a term for it, since one has to intentionally "dllexport" a symbol (otherwise it's what other platforms would call "hidden"). But within a static library, the concept of "dllexported vs. hidden" doesn't apply - all symbols are mixed together with no isolation.

jzmaddock commented 2 years ago

Seems like a perfectly reasonable request, I guess we would just have to macro-ise the namespace open and closing?

ckormanyos commented 2 years ago

perfectly reasonable request, I guess we would just have to macro-ise the namespace open and closing?

Yes. I have done something like this in other projects. Basically, you provide a macro which can supply an outer namespace (which can also be a C++17 nested namespace).

You then need to define separate _BEGIN and _END macros.

The big change I found, however, was in testing. Since you will also want to test some tests with/without the outer namespace, you might end up needing to change a lot of test code to either be using the outer namespace(s) or fully supply the outer namespace resolution. Perhaps my experience was wrong or somehow incomplete, but I found the big effort in re-tooling the mountain of test code.

CaseyCarter commented 2 years ago

Seems like a perfectly reasonable request, I guess we would just have to macro-ise the namespace open and closing?

Yes, that's what I had in mind. My precise use case could be addressed by using a macro for the top-level namespace name (namespace BOOST_MATH_TOP_LEVEL { /* ... */ }) but more generally I can imagine folks wanting to nest within an arbitrary namespace. This is best achieved with "open math namespace from the global namespace", "close math namespace", and "fully-qualified name of math namespace" macros. (The latter is necessary if you need to specialize templates in another namespace - say std - for math types. I'm not sure if Math does this today, but it probably will eventually.)

One design choice to be made is whether the open/close macros include the inner-most braces or not. In other words, do you want:

BOOST_MATH_BEGIN // default definition "namespace boost {"
template <class T> class potato;
BOOST_MATH_END // default definition "}"
namespace std {
template <class T>
struct numeric_limits<BOOST_MATH_PATH::potato<T>> { /* ... */ }; // default definition "::boost"
}

or

BOOST_MATH_BEGIN { // default definition "namespace boost"
    template <class T> class potato;
} BOOST_MATH_END // default definition ""
namespace std {
    template <class T>
    struct numeric_limits<BOOST_MATH_PATH::potato<T>> { /* ... */ }; // default definition "::boost"
}

I believe the former is more common, but the latter is nice for those of us who like to format with indents in namespaces =)

jzmaddock commented 2 years ago

I would tend to weakly prefer the latter.

ckormanyos commented 2 months ago

Hi @jzmaddock I would like to start giving this one a try. I can maybe handle it.

But we need to decide a few things.

This is how I have done this kind of think in the past for some of my own header-only libraries.

So then if the client defines BOOST_MATH_OUTER_NAMESPACE with a top-level line such as:

#define BOOST_MATH_OUTER_NAMESPACE=my_stl_specfun

then the real namespace for ::boost::math will become ::my_stl_specfun::boost::math. Or is a different mechanism in planing.

Cc: @CaseyCarter and @mborland and @NAThompson

jzmaddock commented 2 months ago

I had assumed (perhaps incorrectly) that we would maybe use something like:

#define BOOST_MATH_NAMESPACE_START namespace boost{ namespace math{
#define BOOST_MATH_NAMESPACE_END }}
ckormanyos commented 2 months ago

I had assumed (perhaps incorrectly) that we would maybe use something like:

You would have to only define thos if they were undefined, thus giving the client a chance to define them upstream? I don't really like it because then the entire identity of ::boost::math gets somehow, ... lost. I would actually prefer a client-configurable prefix or pre-pended namespace so at least you always know of the Boost origin/legacy.

ckormanyos commented 2 months ago

But (thinking out loud) would a prepended namespace give the client the desired isolation?

ckormanyos commented 2 months ago

would a prepended namespace give the client the desired isolation?

I think it would isolate beacuse you're basically saying, OK ::boost is a popular, widely-used namespace, let's make sure that a particular standalone specialization of math can be put into ::whatever::boost

ckormanyos commented 2 months ago

Do we need to do anything particular in order to ensure that Multiprecision functions can still be found via ADL?

jzmaddock commented 2 months ago

But (thinking out loud) would a prepended namespace give the client the desired isolation?

I assume that anything that changes the name mangling would work equally well.

Do we need to do anything particular in order to ensure that Multiprecision functions can still be found via ADL?

As long as the namespace is boost::math in the normal case I don't see we need to worry about this?

frederick-vs-ja commented 1 month ago

Will it be sufficient to just replace the math part?


I'm trying to resolve this in Boost.Math by adding the BOOST_MATH_NAMESPACE macro

#ifndef BOOST_MATH_NAMESPACE
#define BOOST_MATH_NAMESPACE math
#endif

and then replace every occurrence of math which is a namespace with BOOST_MATH_NAMESPACE.

With the changes above, MSVC STL's special_math.cpp can define BOOST_MATH_NAMESPACE as _Math (or some other _Uglified identifier, e.g. __math__) before including <boost/math/*> and then use ::boost::BOOST_MATH_NAMESPACE::* to get rid of non-_Uglified symbols.

But the changes need to touch 500+ files in Boost.Math and make maintenance a bit harder (every namespace math should be spelt as BOOST_MATH_NAMESPACE later). Not sure whether this approach is OK.

CaseyCarter commented 1 month ago

Will it be sufficient to just replace the math part?

Assuming every symbol defined by the library is currently in boost::math, I suppose it might suffice. I'd nonetheless prefer the fully general ability to change the entire namespace rather than just a suffix.

Purely aesthetically, I would rather write ::BOOST_MATH_NAMESPACE::meow(args) than ::boost::BOOST_MATH_NAMESPACE::meow(args), but both are admittedly pretty ugly and I'd probably define another macro for this internal use.

ckormanyos commented 1 month ago

I prefer an outer namespace and have had good results in other projects (non-boost) with that philosophy. Also these other projects heavily use ADL. So I'd go with the outer namespace

jzmaddock commented 1 month ago

Personally, I would make BOOST_MATH_NAMESPACE equivalent to boost::math.

I have no great objection to the changes required, but it does increase the number of configurations we need to test quite a bit.