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

Address bind_handler ambiguous overload error when boost/bind/std_placeholders.hpp is included #2638

Conversation

ednolan
Copy link
Contributor

@ednolan ednolan commented Feb 15, 2023

Previously, the implementation of bind_handler made the assumption that the trait boost::is_placeholder would be false for types corresponding to values in the std::placeholders namespace.

However, boost/bind commit c85b31e3d200dda2a73cf0027a82c6d8e29066f8, Support use of standard placeholders with boost::bind, added a new header, boost/bind/std_placeholders.hpp, which adds specializations to
boost::is_placeholder for std::placeholders types.

When this header is included before a use of boost::bind_handler, it results in compiler errors like the following, due to an internal helper function having overloads for
enable_if<boost::is_placeholder...> and
enable_if<std::is_placeholder...>, which were previously assumed to be mutually exclusive, but for which both conditions now are true:

../../../boost/beast/core/detail/bind_handler.hpp:126:18: error: call of overloaded 'extract(std::_Placeholder<1>, boost::beast::detail::tuple<int&&>)' is ambiguous
  126 |         h(extract(detail::get<S>(std::move(args)),
      |           ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  127 |             std::forward<ValsTuple>(vals))...);
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This header is included in boost/bind/bind.hpp, and is transitively included by headers in many boost libraries including:

algorithm
asio
atomic
graph
msm
multi_index
property_tree
ptr_container
python
signals2
test
thread

Making it possible that an include from one of these libraries will randomly cause bind_handler to fail to compile.

This change addresses the issue by ensuring that boost/bind/std_placeholders.hpp is included in the bind_handler implementation file, and adjusting the SFINAE for the boost::is_placeholder overload so that it triggers only
if std::is_placeholder is false. This approach is necessary over simply removing the std::is_placeholder overload because std_placeholders.hpp might have had its functionality suppressed by feature guards. This change also explicitly adds a boost/bind/std_placeholders.hpp include to the bind_handler unit test file to prevent this issue from regressing.

…ceholders.hpp is included

Previously, the implementation of bind_handler made the assumption
that the trait boost::is_placeholder would be false for types
corresponding to values in the std::placeholders namespace.

However, boost/bind commit c85b31e3d200dda2a73cf0027a82c6d8e29066f8,
`Support use of standard placeholders with boost::bind`, added a new
header, boost/bind/std_placeholders.hpp, which adds specializations to
 boost::is_placeholder for std::placeholders types.

When this header is included before a use of boost::bind_handler, it
results in compiler errors like the following, due to an internal
helper function having overloads for
enable_if<boost::is_placeholder...> and
enable_if<std::is_placeholder...>, which were previously assumed to be
mutually exclusive, but for which both conditions now are true:

../../../boost/beast/core/detail/bind_handler.hpp:126:18: error: call of overloaded 'extract(std::_Placeholder<1>, boost::beast::detail::tuple<int&&>)' is ambiguous
  126 |         h(extract(detail::get<S>(std::move(args)),
      |           ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  127 |             std::forward<ValsTuple>(vals))...);
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This header is included in boost/bind/bind.hpp, and is transitively
included by headers in many boost libraries including:

algorithm
asio
atomic
graph
msm
multi_index
property_tree
ptr_container
python
signals2
test
thread

Making it possible that an include from one of these libraries will
randomly cause bind_handler to fail to compile.

This change addresses the issue by eliminating one of the ambiguous
overloads and ensuring that boost/bind/std_placeholders.hpp is
included in the bind_handler implementation file, so that the
enable_if<boost::is_placeholder...> overload can handle both
boost::placeholders and std::placeholders values. It also explicitly
adds a boost/bind/std_placeholders.hpp include to the bind_handler
unit test to prevent this issue from regressing.
@vinniefalco
Copy link
Member

Thank you for your contribution!

boost/bind/std_placeholders.hpp has the following feature guards:

if !defined(BOOST_NO_CXX11_HDR_FUNCTIONAL) && !defined(BOOST_NO_CXX11_DECLTYPE) && !defined(BOOST_NO_CXX11_HDR_TYPE_TRAITS)

If this check fails, it will not detect std::placeholders values as
placeholders, making the previous solution fail.

This update restores the std::is_placeholder overload and adjusts the
SFINAE for the boost::is_placeholder overload so that it triggers only
if std::is_placeholder is false, making bind_handler work regardless
of whether std_placeholders.hpp's feature guard applies.
@cppalliance-bot
Copy link

pullrequest
Timeline tracing charts: https://2638.beast.tracing.cppalliance.org/index.html

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #2638 (98b681b) into develop (341ac75) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2638   +/-   ##
========================================
  Coverage    92.93%   92.93%           
========================================
  Files          177      177           
  Lines        13651    13651           
========================================
  Hits         12687    12687           
  Misses         964      964           
Impacted Files Coverage Δ
include/boost/beast/core/detail/bind_handler.hpp 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 341ac75...98b681b. Read the comment docs.

@klemens-morgenstern
Copy link
Collaborator

Looks good; but can't we just include the std_placeholders.hpp and just add one function? Or do you want to keep the headers to a minumum @vinniefalco ?

@vinniefalco
Copy link
Member

That's up to you. Don't break anything.

@ednolan
Copy link
Contributor Author

ednolan commented Feb 16, 2023

can't we just include the std_placeholders.hpp and just add one function

If what you mean by this is, can't we have a single overload handling the std::placeholders and boost::placeholders cases: that was my original approach. But the problem is that std_placeholders.hpp isn't guaranteed to actually work for std::placeholders because it can be turned off by feature gating. This caused my original commit to fail CI for GCC 4. So you still need two overloads.

@klemens-morgenstern klemens-morgenstern merged commit 8dc6daa into boostorg:develop Feb 17, 2023
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.

None yet

5 participants