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

Errors with string_view since update to 1.81.0 #2594

Closed
jcmonnin opened this issue Dec 19, 2022 · 7 comments · Fixed by #2819
Closed

Errors with string_view since update to 1.81.0 #2594

jcmonnin opened this issue Dec 19, 2022 · 7 comments · Fixed by #2819
Assignees
Labels
Doc A documentation specific issue

Comments

@jcmonnin
Copy link

When updating to boost 1.81.0, I faced some compile errors in my code along with deprecation warnings

BOOST_BEAST_USE_STD_STRING_VIEW is deprecated, use BOOST_NO_CXX17_HDR_STRING_VIEW instead

This links to #2451.
Looking into that, it looks like beast now uses always boost::core::string_view which seems to be a separate string_view implementation (with some implicit converstions to std::string_view). As it's in details, it's not documented.

Loosing the ability to directly use std::string_view looks like a step back to me. Some things that used to work don't work anymore, like implicitly assigning a string_view to a std::filesystem::path. Obviously this is nothing that can't be worked around, but this illustrates why having multiple string_view implementations isn't necessarily a good idea.

The deprecation warning seems also a bit misleading. BOOST_NO_CXX17_HDR_STRING_VIEW seems to do the opposite to BOOST_BEAST_USE_STD_STRING_VIEW (eg. completely switch off conversions to std::string_view).

Please correct me if I got something wrong.

Would you consider bringing back the option to use std::string_view?

@klemens-morgenstern
Copy link
Collaborator

The idea was to unify this, because otherwise it gets weird. With the header set, boost::core::string_view can interact with std::string_view, but it doesn't work the other way.

The problem is not only that beast is a C++11 library and std::string_view C++17, it's also that the standard versions tend to become stale; e.g. boost::system::error_code now has a few nifty features not available with std::error_code. This is why there was an effort to unify towards the boost types.

I am surprised std::filesystem::path can't be constructed from a boost::core::string_view, tbh.

@vinniefalco what do you think?

@jcmonnin
Copy link
Author

I see your point about the standard libraries becoming stale from the point of view of the library author. For the specific case of std::string_view , if it's missing important features and there are plans to have bring a improved version, this is understandable. In other cases, having boost::core::string_view defined as a type alias of std::string_view when targeting C++17 or higher is going to give a better user experience (both to interact with other libraries and in the debugger).

As a library user targeting C++20, I prefer using the standard library where possible and see the boost libraries as one source of libraries to provide me the features I need (here mostly networking); similar to any other library on github. I that sense I would prefer if boost's effort for fallback implementations for old C++ standards and improved versions of existing standard library code were in a separate project. I understand this is personal opinion and doesn't align with the general direction of the boost project.

Btw, asio and beast are great!

@vinniefalco
Copy link
Member

@vinniefalco what do you think?

I prefer not to have the API/ABI of the library change based on a macro.

@jcmonnin
Copy link
Author

Feel free to close the issue, unless you think it's worth keeping it open for one of the following points:

  1. Deprecation warning is misleading (a better message would be something like BOOST_BEAST_USE_STD_STRING_VIEW is deprecated, boost::core::string_view is always used
  2. Beast uses undocumented private type std::core::string_view (in boost/core/detail/string_view.hpp) in it's public interface.
  3. Cannot construct std::filesystem::path from a boost::core::string_view (this is an issue that belongs to boost/core)

About 2) I saw #110, saying:

Also, it would be a good idea to mention that boost::core::string_view is not intended to be
used by users (hence it is in detail) and is supposed to be used by Boost library writers.

and following the link there, I saw some comment in an older version of the source file

Unlike `boost::string_view`, `boost::core::string_view` has implicit
conversions from and to `std::string_view`, which allows Boost libraries that
support C++11/C++14 to use it in interfaces without forcing users to forgo the
use of `std::string_view` in their code.

Previously, I was using beast::string_view in my code when interfacing to beast for consistency. I changed all the beast::string_view in my code to std::string_view and this fixed the compile errors. I guess the missing documentation and description of the design rationale of boost::core::string_view is not ideal.

@heitbaum
Copy link

With this change and the deprecation of to_string() - snapcast fails to compile.

https://github.com/badaix/snapcast/blob/e30a9f335badc81e743c61ceda6ef2dfc698573b/server/control_session_http.cpp#L127-L185

is the fix to cast static_cast<std::string>(sv)?
Or is there a better way to fix this?

https://www.learncpp.com/cpp-tutorial/introduction-to-stdstring_view/

@vinniefalco vinniefalco added the Doc A documentation specific issue label Dec 21, 2022
@vinniefalco
Copy link
Member

The documentation needs to be updated, sorry for this confusion!

@jcmonnin
Copy link
Author

Thanks Vinnie and Klemens for all your work. Unfortunately I'm a bit too far away from the code to be able to send a PR for the doc.

bazaah added a commit to bazaah/aur-ceph that referenced this issue Mar 7, 2023
So long story short, boost 1.81 has fairly large breaking API changes,
as they migrated to use an internal string_view shim rather than
std::string_view, across the entire beast codebase. Therefore, we
remove any attempts to convert to std::string_view from
boost::core::string_view, as it implicitly handles conversions to
std::string where needed (like std::string_view).

Long story. boostorg/beast#2451 introduced a change in their string_view
type defs for beast, which ultimately removed the to_string() method
inherited from boost::string_view (which is different to boost::core),
effectively changing the type of most of the returned values in
rgw_asio_client.cc's header manipulation logic.

However, this new shim string_view implicitly converts to std::string,
which is needed by RGWEnv::set, hence our removal of the various
to_string()s scattered across the file.

Notably, RGWEnv::set is remarkably strict in its accepted values, so I
trust that if this compiles we have introduced a subtle use after free
vis-a-vis all these string_views flying around.

References: boostorg/beast#2451
References: https://github.com/ceph/ceph/blob/v17.2.5/src/rgw/rgw_asio_client.cc
References: boostorg/beast#2594 (comment)
References: https://github.com/ceph/ceph/blob/v17.2.5/src/rgw/rgw_env.cc#L22
ashtum added a commit to ashtum/beast that referenced this issue Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Doc A documentation specific issue
Projects
No open projects
Status: Closed
Development

Successfully merging a pull request may close this issue.

4 participants