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

Convert code base from boost::string_view to std::string_view #3435

Open
carlhua opened this issue Jun 3, 2020 · 4 comments
Open

Convert code base from boost::string_view to std::string_view #3435

carlhua opened this issue Jun 3, 2020 · 4 comments
Assignees
Labels
RIPD Export Exported from legacy JIRA issue tracking Tech Debt Non-urgent improvements

Comments

@carlhua
Copy link
Contributor

carlhua commented Jun 3, 2020

Issue Description

The codebase currently does not make a lot of use of string_view. Mostly it shows up in the asio facing parts of the code. I tried converting those uses of boost::string_view to std::string_view a few months ago. It almost worked.

There’s a problem in boost 1.70 where in Windows the flag that tells beast to use std::string_view (not boost::string_view) does not work correctly. I understand that particular problem is fixed in Boost 1.71. So once we get to a place where the code base requires boost 1.71 or greater, then I think we can take on this task.

Once we have std::string_view in the code base there are a number of optimizations that can be performed. But I believe it should all start by moving to std::string_view first.

Exported from RIPD-1789

@carlhua carlhua added Tech Debt Non-urgent improvements RIPD Export Exported from legacy JIRA issue tracking labels Jun 3, 2020
@intelliot
Copy link
Collaborator

@ckeshava could you think about this and let us know your recommendation?

Feel free to consult with @scottschurr as needed (he opened the original issue)

@ckeshava
Copy link
Collaborator

Hello,
As per my understanding, we are using Boost v1.77. So we should not run into the issue pointed out earlier. I will test for compatibility on Visual Studio 2019 after making these changes.

I believe std::string_view is almost always better than std::string const&. Do I need to think about other alternatives for boost::string_view? What do you think @scottschurr ?

@scottschurr
Copy link
Collaborator

Hi @ckeshava. Thanks for looking into this.

In terms of changes, my feeling is that, wherever we can, we should be replacing boost::string_view with std::string_view. The places that had problems before were boost::asio and boost::beast. Hopefully fixing those is now possible. I also had problems with soci wanting to use boost::string_view instead of std::string_view. It would be interesting to learn the state of the world on that front as well.

In terms of replacing std::string with std::string_view, that needs to be done very carefully with an eye toward lifetimes. So if we want to address that (and there are definitely places where that replacement is a win), that kind of change would want to be in one or more separate commits from the one described in the previous paragraph (just changing string_view types).

In case it's helpful, here is an outstanding review of what should (and should not) be done with string_view: https://www.youtube.com/watch?v=H9gAaNRoon4

I hope that helps. Let me know if you have further questions.

@thejohnfreeman
Copy link
Collaborator

We will need to finish eliminating string_views other than std::string_view if we want to upgrade to Boost 1.83 or later. The BEAST_USE_STD_STRING_VIEW macro that we have been relying on was removed in 1.83. We already removed boost::string_view in #4509, but 15 occurrences of boost::beast::string_view remain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RIPD Export Exported from legacy JIRA issue tracking Tech Debt Non-urgent improvements
Projects
None yet
Development

No branches or pull requests

5 participants