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

Fix #include inside boost namespace #670

Merged
merged 1 commit into from
Aug 2, 2021
Merged

Conversation

dscharrer
Copy link
Contributor

The existing code fails to build if was not already included.

The existing code fails to build if <utility> was not already included.
@jwakely
Copy link
Contributor

jwakely commented Aug 2, 2021

This needs to be fixed in a 1.76.1 release because it's currently totally broken with GCC trunk. I've refactored some libstdc++ headers so that the whole of <utility> is included less often by other headers in the library. Now it doesn't get implicitly included before reaching this bug, and so the first time it gets included is in the wrong namespace.

@jwakely
Copy link
Contributor

jwakely commented Aug 2, 2021

The #include inside the boost::math namespace is obviously totally broken, but so is testing the __cpp_lib_integer_sequence feature test macro before including <utility>. Unless you include <version> (which isn't present on older implementations) then the only guaranteed way to get the macro is by including <utility>. So using it as the condition to decide whether or not to include <utility> is backwards.

You could do:

#ifdef __has_include
# if __has_include(<version>)
#  include <version>
#  define BOOST_MATH_HAVE_VERSION_HDR 1
#endif 
#endif 

#ifdef BOOST_MATH_HAVE_VERSION_HDR
# include <version>
#else
# include <utility>
#endif

....

#ifdef __cpp_lib_integer_sequence

But frankly, you might as well just include <utility>.

@jwakely jwakely mentioned this pull request Aug 2, 2021
@jzmaddock
Copy link
Collaborator

@mborland ?

I'm wondering how this managed to slip through CI testing as well.

@jwakely
Copy link
Contributor

jwakely commented Aug 2, 2021

I'm wondering how this managed to slip through CI testing as well.

Possible scenarios:

  • <utility> has already been included before that point, so the erroneous #include <utility> is a no-op thanks to its header guard. The bug doesn't show up.
  • <utility> has not been included yet, but that means the __cpp_lib_integer_sequence macro isn't defined, so you don't try to #include <utility> anyway. The bug doesn't show up.
  • <utility> has not been included, but some internal std::lib header which happens to define __cpp_lib_integer_sequence has been previously included. That means the macro is defined, so you try to #include <utility> inside the boost namespace. The bug shows up.

Until last week, the third scenario wouldn't happen with libstdc++, because std::integer_sequence and its feature test macro were defined in <utility> itself. The way the macro got defined was by including that header, which means the erroneous #include is a no-op. Since last week, std::integer_sequence is in a new internal <bits/xxx.h> header that is used by <utility> but also some other headers such as <tuple>, so the third scenario is now possible.

Presumably the third scenario also didn't happen with other implementations in your tests, although I suspect this would have triggered the bug using any impl:

#include <version>
#include <boost/math/tools/mp.hpp>

It certainly fails with old versions of libstdc++ and with a recent libc++.

@jwakely
Copy link
Contributor

jwakely commented Aug 2, 2021

A workaround for this bug is to #include <utility> before any use of boost 1.76.0 headers.

@mborland
Copy link
Member

mborland commented Aug 2, 2021

@jzmaddock Fortunately master for 1.77 is open until Wednesday so this fix is definitely worth cherry-picking.

@jzmaddock
Copy link
Collaborator

Test case is:

#include <version>
#include <boost/math/tools/mp.hpp>

Note that if we replace boost/math/tools/mp.hpp with either of the public headers which include it: boost/math/differentiation/autodiff.hpp or boost/math/policies/policy.hpp then I don't see the issue (with gcc-10 at least), as these include albeit inadvertently via . So it's not quite as grievous as it first appears, but does need fixing ASAP.

@jzmaddock
Copy link
Collaborator

Failing CI are server network issues, merging...

@jzmaddock jzmaddock merged commit 3e46eba into boostorg:develop Aug 2, 2021
@pdimov
Copy link
Member

pdimov commented Aug 2, 2021

index_sequence seems to be defined twice.

template<std::size_t... I>
using index_sequence = integer_sequence<std::size_t, I...>;

and
template<std::size_t... I>
using index_sequence = integer_sequence<std::size_t, I...>;

@jwakely
Copy link
Contributor

jwakely commented Aug 2, 2021

then I don't see the issue (with gcc-10 at least), as these include albeit inadvertently via .

(N.B. there's an HTML escaping issue here)

Right, but with GCC's current trunk it fails much more readily, because <algorithm> no longer includes <utility> e.g. you get errors for realistic use cases like:

#include <boost/math/distributions/hypergeometric.hpp>
boost::math::hypergeometric_distribution dist(50, 50, 500);

Admittedly GCC trunk isn't going to be released until Q2 2022, but it's likely to start appearing in distros sooner than that (maybe late 2021).

@jzmaddock
Copy link
Collaborator

index_sequence seems to be defined twice.

You're correct - good catch, duplicate typedefs are allowed, but we shouldn't do that, will submit a fresh PR for that ASAP.

Right, but with GCC's current trunk it fails much more readily, because no longer includes e.g. you get errors for realistic use cases like:

Nod. It's asking for trouble.

@jzmaddock
Copy link
Collaborator

index_sequence seems to be defined twice.

See #671

ashley-b added a commit to ashley-b/conan-center-index that referenced this pull request Feb 3, 2023
- The existing code fails to build if <utility> was not already included.
- boostorg/math#670
conan-center-bot pushed a commit to conan-io/conan-center-index that referenced this pull request Feb 9, 2023
* Boost/1.76.0: Fix #include inside boost namespace patch

- The existing code fails to build if <utility> was not already included.
- boostorg/math#670

* Apply suggestions from code review

Co-authored-by: Jordan Williams <jordan@jwillikers.com>

---------

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Co-authored-by: Jordan Williams <jordan@jwillikers.com>
sabelka pushed a commit to sabelka/conan-center-index that referenced this pull request Feb 12, 2023
…/1.77.0)

* Boost/1.76.0: Fix #include inside boost namespace patch

- The existing code fails to build if <utility> was not already included.
- boostorg/math#670

* Apply suggestions from code review

Co-authored-by: Jordan Williams <jordan@jwillikers.com>

---------

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Co-authored-by: Jordan Williams <jordan@jwillikers.com>
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.

5 participants