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

Update codebase from boost::string_view into std::string_view #4509

Merged
merged 33 commits into from
Jun 17, 2024
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
ba1da15
Migrated boost::string_view to std::string_view
ckeshava Apr 24, 2023
3850093
apply git-clang-format patch file from Github Actions
ckeshava Apr 24, 2023
1141e0f
use std::string_view.remove_suffix() member function to clear the val…
ckeshava Apr 24, 2023
c966149
[FOLD] address PR comments from Scott S and Nik
ckeshava Jun 23, 2023
efaf7ae
replace usages of boost::string_view with std::string_view. In turn, …
ckeshava Sep 11, 2023
92fda38
Merge branch 'develop' of https://github.com/XRPLF/rippled into updat…
ckeshava Sep 11, 2023
8c17a5c
clang format
ckeshava Sep 11, 2023
eada2c3
fix compiler errors due to the ServerHandlerImp -> ServerHandler change
ckeshava Sep 11, 2023
4967121
Merge branch 'develop' into updateStringView
ckeshava Jan 5, 2024
6dbfd7f
Merge branch 'develop' into updateStringView
ckeshava Jan 5, 2024
e4015fd
Update src/ripple/overlay/impl/Handshake.cpp
ckeshava Jan 5, 2024
be6e9be
removed old comment and an irrelevant file
ckeshava Jan 6, 2024
9d9f40e
partially addressed John's comments: Use std::string_view instead of …
ckeshava Jan 6, 2024
0c39f13
Merge branch 'updateStringView' of https://github.com/ckeshava/ripple…
ckeshava Jan 6, 2024
8da02e5
introduce template specialization for beast::LexicalCast<Out, std::st…
ckeshava Jan 6, 2024
a433207
refactor PeerImp::parseLedgerHash and base64_decode to use std::strin…
ckeshava Jan 6, 2024
2cf0eb8
refactor parseBase58 overloads to use std::string_view instead of std…
ckeshava Jan 6, 2024
85e001d
address John's comments
ckeshava Jan 8, 2024
88412ea
include BOOST_BEAST_USE_STD_STRING macro in the build system. Hence, …
ckeshava Jan 8, 2024
b780312
remove unused include statements
ckeshava Jan 8, 2024
999d9c2
build instructions to configure boost to use std::string_view
ckeshava Jan 8, 2024
cee5ff8
address John's PR comments: Update the Build instructions, include a …
ckeshava Jan 16, 2024
ac77412
Merge branch 'develop' into updateStringView
ckeshava Jan 16, 2024
cfa960f
replace the usage of std::string_view const& to std::string_view (pas…
ckeshava Jan 26, 2024
5313b13
Merge branch 'develop' into updateStringView
ckeshava Jan 26, 2024
204f9f4
construct a std:;string instead of using the assignment operator
ckeshava Feb 2, 2024
d851b16
Merge branch 'develop' into updateStringView
ckeshava Feb 2, 2024
49968bf
do not return a std::string_view from functions for safety reasons - …
ckeshava Feb 22, 2024
a20907f
Merge branch 'develop' into updateStringView
ckeshava Feb 22, 2024
d3d309a
Merge branch 'develop' into updateStringView
ckeshava Feb 28, 2024
fcf1375
Merge branch 'develop' into updateStringView
ckeshava Mar 6, 2024
3cda316
- rectify the mistakes in the merge from base58 changes.
ckeshava Mar 7, 2024
ba99ab4
Merge branch 'develop' into updateStringView
seelabs Jun 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions BUILD.md
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,20 @@ then you will need to choose the `libstdc++11` ABI:
conan profile update settings.compiler.libcxx=libstdc++11 default
```


Ensure inter-operability between `boost::string_view` and `std::string_view` types:

```
conan profile update 'conf.tools.build:cxxflags+=["-DBOOST_BEAST_USE_STD_STRING_VIEW"]' default
conan profile update 'env.CXXFLAGS="-DBOOST_BEAST_USE_STD_STRING_VIEW"' default
```

If you have other flags in the `conf.tools.build` or `env.CXXFLAGS` sections, make sure to retain the existing flags and append the new ones. You can check them with:
```
conan profile show default
```


**Windows** developers may need to use the x64 native build tools.
An easy way to do that is to run the shortcut "x64 Native Tools Command
Prompt" for the version of Visual Studio that you have installed.
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/app/misc/ValidatorList.h
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,7 @@ class ValidatorList
*/
std::optional<Json::Value>
getAvailable(
boost::beast::string_view const& pubKey,
std::string_view pubKey,
std::optional<std::uint32_t> forceVersion = {});

/** Return the number of configured validator list sites. */
Expand Down
6 changes: 3 additions & 3 deletions src/ripple/app/misc/impl/ValidatorList.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
#include <boost/regex.hpp>

#include <cmath>
#include <mutex>
#include <numeric>
#include <shared_mutex>

Expand Down Expand Up @@ -213,7 +212,8 @@ ValidatorList::load(
return false;
}

auto const id = parseBase58<PublicKey>(TokenType::NodePublic, match[1]);
auto const id =
parseBase58<PublicKey>(TokenType::NodePublic, match[1].str());

if (!id)
{
Expand Down Expand Up @@ -1676,7 +1676,7 @@ ValidatorList::for_each_available(

std::optional<Json::Value>
ValidatorList::getAvailable(
boost::beast::string_view const& pubKey,
std::string_view pubKey,
std::optional<std::uint32_t> forceVersion /* = {} */)
{
std::shared_lock read_lock{mutex_};
Expand Down
4 changes: 2 additions & 2 deletions src/ripple/basics/StringUtilities.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ strUnHex(std::string const& strSrc)
}

inline std::optional<Blob>
strViewUnHex(boost::string_view const& strSrc)
strViewUnHex(std::string_view strSrc)
{
return strUnHex(strSrc.size(), strSrc.cbegin(), strSrc.cend());
}
Expand Down Expand Up @@ -150,7 +150,7 @@ to_uint64(std::string const& s);
doesn't check whether the TLD is valid.
*/
bool
isProperlyFormedTomlDomain(std::string const& domain);
isProperlyFormedTomlDomain(std::string_view domain);

} // namespace ripple

Expand Down
2 changes: 1 addition & 1 deletion src/ripple/basics/base64.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ base64_encode(std::string const& s)
}

std::string
base64_decode(std::string const& data);
base64_decode(std::string_view data);

} // namespace ripple

Expand Down
4 changes: 2 additions & 2 deletions src/ripple/basics/impl/StringUtilities.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ to_uint64(std::string const& s)
}

bool
isProperlyFormedTomlDomain(std::string const& domain)
isProperlyFormedTomlDomain(std::string_view domain)
{
// The domain must be between 4 and 128 characters long
if (domain.size() < 4 || domain.size() > 128)
Expand All @@ -143,7 +143,7 @@ isProperlyFormedTomlDomain(std::string const& domain)
,
boost::regex_constants::optimize);

return boost::regex_match(domain, re);
return boost::regex_match(domain.begin(), domain.end(), re);
}

} // namespace ripple
2 changes: 1 addition & 1 deletion src/ripple/basics/impl/base64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ base64_encode(std::uint8_t const* data, std::size_t len)
}

std::string
base64_decode(std::string const& data)
base64_decode(std::string_view data)
{
std::string dest;
dest.resize(base64::decoded_size(data.size()));
Expand Down
61 changes: 48 additions & 13 deletions src/ripple/beast/core/LexicalCast.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#ifndef BEAST_MODULE_CORE_TEXT_LEXICALCAST_H_INCLUDED
#define BEAST_MODULE_CORE_TEXT_LEXICALCAST_H_INCLUDED

#include <boost/core/detail/string_view.hpp>
#include <algorithm>
#include <cassert>
#include <cerrno>
Expand Down Expand Up @@ -64,9 +65,9 @@ struct LexicalCast<std::string, In>
}
};

// Parse std::string to number
template <class Out>
struct LexicalCast<Out, std::string>
// Parse a std::string_view into a number
template <typename Out>
struct LexicalCast<Out, std::string_view>
{
explicit LexicalCast() = default;

Expand All @@ -78,7 +79,7 @@ struct LexicalCast<Out, std::string>
std::enable_if_t<
std::is_integral_v<Integral> && !std::is_same_v<Integral, bool>,
bool>
operator()(Integral& out, std::string const& in) const
operator()(Integral& out, std::string_view in) const
{
auto first = in.data();
auto last = in.data() + in.size();
Expand All @@ -92,20 +93,23 @@ struct LexicalCast<Out, std::string>
}

bool
operator()(bool& out, std::string in) const
operator()(bool& out, std::string_view in) const
{
std::string result;

// Convert the input to lowercase
std::transform(in.begin(), in.end(), in.begin(), [](auto c) {
return std::tolower(static_cast<unsigned char>(c));
});
std::transform(
in.begin(), in.end(), std::back_inserter(result), [](auto c) {
return std::tolower(static_cast<unsigned char>(c));
});

if (in == "1" || in == "true")
if (result == "1" || result == "true")
{
out = true;
return true;
}

if (in == "0" || in == "false")
if (result == "0" || result == "false")
{
out = false;
return true;
Expand All @@ -114,9 +118,38 @@ struct LexicalCast<Out, std::string>
return false;
}
};

//------------------------------------------------------------------------------

// Parse boost library's string_view to number or boolean value
// Note: As of Jan 2024, Boost contains three different types of string_view
// (boost::core::basic_string_view<char>, boost::string_ref and
// boost::string_view). The below template specialization is included because
// it is used in the handshake.cpp file
template <class Out>
struct LexicalCast<Out, boost::core::basic_string_view<char>>
{
explicit LexicalCast() = default;

bool
operator()(Out& out, boost::core::basic_string_view<char> in) const
{
return LexicalCast<Out, std::string_view>()(out, in);
}
};

// Parse std::string to number or boolean value
template <class Out>
struct LexicalCast<Out, std::string>
{
explicit LexicalCast() = default;

bool
operator()(Out& out, std::string in) const
{
return LexicalCast<Out, std::string_view>()(out, in);
}
};

// Conversion from null terminated char const*
template <class Out>
struct LexicalCast<Out, char const*>
Expand All @@ -126,7 +159,8 @@ struct LexicalCast<Out, char const*>
bool
operator()(Out& out, char const* in) const
{
return LexicalCast<Out, std::string>()(out, in);
assert(in);
return LexicalCast<Out, std::string_view>()(out, in);
}
};

Expand All @@ -140,7 +174,8 @@ struct LexicalCast<Out, char*>
bool
operator()(Out& out, char* in) const
{
return LexicalCast<Out, std::string>()(out, in);
assert(in);
return LexicalCast<Out, std::string_view>()(out, in);
}
};

Expand Down
1 change: 1 addition & 0 deletions src/ripple/json/Output.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

#include <boost/beast/core/string.hpp>
#include <functional>
#include <string>

namespace Json {

Expand Down
5 changes: 2 additions & 3 deletions src/ripple/overlay/impl/Cluster.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
*/
//==============================================================================

#include <ripple/app/main/Application.h>
#include <ripple/basics/Log.h>
#include <ripple/basics/StringUtilities.h>
#include <ripple/core/Config.h>
Expand All @@ -27,7 +26,6 @@
#include <ripple/protocol/jss.h>
#include <ripple/protocol/tokens.h>
#include <boost/regex.hpp>
#include <memory.h>

namespace ripple {

Expand Down Expand Up @@ -113,7 +111,8 @@ Cluster::load(Section const& nodes)
return false;
}

auto const id = parseBase58<PublicKey>(TokenType::NodePublic, match[1]);
auto const id =
parseBase58<PublicKey>(TokenType::NodePublic, match[1].str());

if (!id)
{
Expand Down
13 changes: 4 additions & 9 deletions src/ripple/overlay/impl/Handshake.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,12 @@
#include <ripple/app/ledger/LedgerMaster.h>
#include <ripple/app/main/Application.h>
#include <ripple/basics/base64.h>
#include <ripple/basics/safe_cast.h>
#include <ripple/beast/core/LexicalCast.h>
#include <ripple/beast/rfc2616.h>
#include <ripple/overlay/impl/Handshake.h>
#include <ripple/protocol/digest.h>

#include <boost/regex.hpp>

#include <algorithm>
#include <chrono>

// VFALCO Shouldn't we have to include the OpenSSL
// headers or something for SSL_get_finished?
Expand All @@ -46,8 +42,8 @@ getFeatureValue(
return {};
boost::smatch match;
boost::regex rx(feature + "=([^;\\s]+)");
std::string const value = header->value();
if (boost::regex_search(value, match, rx))
std::string const allFeatures = std::string{header->value()};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to construct then assign to it. You can simply construct it:

std::string const allFeatures(header->value());

The generated code is probably identical, but I think simply constructing the string is easier to read.

if (boost::regex_search(allFeatures, match, rx))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you call the overload of boost::regex_search that takes two iterators, then you can leave the value as a std::string_view.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting observation!

boost provides the following typedefs for the boost::smatch match; variable. We use it to store the appropriate portion of the input. I couldn't find the correct type for this variable. Ideally, I'd need match_results<std::string_view::const_iterator> to be the type of match.

match_results<const char*> is incorrect, because some inputs might not be null-terminated.
match_results<std::string::const_iterator> is type incompatible with std:string_view value;

typedef match_results<const char*> cmatch;
typedef match_results<std::string::const_iterator> smatch;
#ifndef BOOST_NO_WREGEX
typedef match_results<const wchar_t*> wcmatch;
typedef match_results<std::wstring::const_iterator> wsmatch;
#endif

I need to use this overload for the regex_search function.

template <class BidiIterator, class Allocator, class charT, class traits>
bool regex_search(BidiIterator first, BidiIterator last, 
                  match_results<BidiIterator, Allocator>& m, 
                  const basic_regex<charT, traits>& e, 
                  match_flag_type flags,
                  BidiIterator base)

How can I work around this?

return {match[1]};
return {};
}
Expand Down Expand Up @@ -243,7 +239,7 @@ verifyHandshake(
{
std::uint32_t nid;

if (!beast::lexicalCastChecked(nid, std::string(iter->value())))
if (!beast::lexicalCastChecked(nid, iter->value()))
throw std::runtime_error("Invalid peer network identifier");

if (networkID && nid != *networkID)
Expand All @@ -252,8 +248,7 @@ verifyHandshake(

if (auto const iter = headers.find("Network-Time"); iter != headers.end())
{
auto const netTime =
[str = std::string(iter->value())]() -> TimeKeeper::time_point {
auto const netTime = [str = iter->value()]() -> TimeKeeper::time_point {
TimeKeeper::duration::rep val;

if (beast::lexicalCastChecked(val, str))
Expand Down
7 changes: 3 additions & 4 deletions src/ripple/overlay/impl/OverlayImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
#include <ripple/server/SimpleWriter.h>

#include <boost/algorithm/string/predicate.hpp>
#include <boost/utility/in_place_factory.hpp>

namespace ripple {

Expand Down Expand Up @@ -829,7 +828,7 @@ OverlayImpl::getOverlayInfo()
auto version{sp->getVersion()};
if (!version.empty())
// Could move here if Json::value supported moving from strings
pv[jss::version] = version;
pv[jss::version] = std::string{version};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good place here for that operator= (std::string_view const&) overload I mentioned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm okay

}

std::uint32_t minSeq, maxSeq;
Expand Down Expand Up @@ -997,9 +996,9 @@ OverlayImpl::processValidatorList(
return true;
};

auto key = req.target().substr(prefix.size());
Copy link
Contributor

@nbougalis nbougalis Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd personally leave this alone. You're adding complexity for no real reason here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I leave type-deduction to auto, I'm not 100% sure if it is deduced as std::string_view. On line 1006, there is a comparison of key with std::string_view::npos.

My observation is that construction of std::string_view without the second length parameter results in garbage values at the end of key. I'm not really sure why that's happening.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sympathize with your position, @nbougalis. But key is passed to ValidatorList::getAvailable(), which accepts a std::string_view. The conversion must happen somewhere.

I think perhaps the conversion can be done a bit more cleanly like this:

    // Convert the boost::string_view, returned by
    // boost::http::request::target(), into a std::string_view.
    std::string_view key = [&req, &prefix]() {
        boost::string_view const key = req.target().substr(prefix.size());
        return std::string_view(key.data(), key.length());
    }();

Since the auto is gone we can always see what we're handling. And it limits the scope of the boost::string_view as much as possible. But that doesn't remove the fundamental objection.

FWIW, I have a theory regarding the garbage values at the end of key if the length parameter is omitted. Remember that a string_view is a pointer and a length. There is no guarantee of a null termination to the string of characters contained by a string_view. This is one of the things that can make string_view dangerous to use.

If you construct a string_view with only a char*, then the string_view constructor must determine the length of the string it represents. It does so by scanning for a null termination. Since the old string_view you are using to construct the new string_view is not required to contain a null terminated string, the constructor of the new string_view may walk past the end of valid characters, potentially resulting in undefined behavior.

For more details see the description of constructor 4 on this page: https://en.cppreference.com/w/cpp/string/basic_string_view/basic_string_view

The lesson from this is that the only situation where a string_view should be constructed with a char* (and no length) is if you know the char* is a c-style string with a null termination.

string_view is often a nice optimization. But it's easy to mess up with it. We must handle the knife carefully.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i missed this comment, thanks for the explanation Scott! yes, that makes sense

std::string_view key = req.target().substr(prefix.size());

if (auto slash = key.find('/'); slash != boost::string_view::npos)
if (auto slash = key.find('/'); slash != std::string_view::npos)
{
auto verString = key.substr(0, slash);
if (!boost::conversion::try_lexical_convert(verString, version))
Expand Down
Loading
Loading