Bug #39619
closedcleanup: replace boost::string_ref/view with c++17 std::string_view
0%
Updated by Matthew Oliver over 4 years ago
Seems pretty straight forward. I'll give this a go.. famous. last. words :P
Updated by Matthew Oliver over 4 years ago
I've done the change over, but it seems rgw_acl fails. When compiled with c++17, sure there is std::string_view but it seems std::basic_string_view doesn't yet contain starts_with or ends_with. These aren't added until c++20 (if I'm reading cppreference.com write (https://en.cppreference.com/w/cpp/string/basic_string_view).
Where as boost::basic_string_view does. I.e:
/ceph/src/rgw/rgw_acl.h: In member function ‘bool ACLReferer::is_match(std::string_view) const’: /ceph/src/rgw/rgw_acl.h:237:25: error: ‘const class std::basic_string_view<char>’ has no member named ‘ends_with’ return http_host->ends_with(url_spec); ^~~~~~~~~ /ceph/src/rgw/rgw_acl.h: In member function ‘boost::optional<std::basic_string_view<char> > ACLReferer::get_http_host(std::string_view) const’: /ceph/src/rgw/rgw_acl.h:260:46: error: ‘const string_view {aka const class std::basic_string_view<char>}’ has no member named ‘starts_with’ if (pos == std::string_view::npos || url.starts_with("://") || ^~~~~~~~~~~ /ceph/src/rgw/rgw_acl.h:261:13: error: ‘const string_view {aka const class std::basic_string_view<char>}’ has no member named ‘ends_with’ url.ends_with("://") || url.ends_with('@')) { ^~~~~~~~~ /ceph/src/rgw/rgw_acl.h:261:37: error: ‘const string_view {aka const class std::basic_string_view<char>}’ has no member named ‘ends_with’ url.ends_with("://") || url.ends_with('@')) { ^~~~~~~~~
So we could attempt to have a mix of it all, but that might not be very pretty and could be time bomb (having to use the right view) until we can default to c++20.
Another option is to rework rgw_acl to not use starts_with/ends_with.
Updated by Matthew Oliver over 4 years ago
oh maybe I can just use something like: #include <boost/algorithm/string/predicate.hpp>
Which will enable:
boost::starts_with()
etc.
Updated by Matthew Oliver over 4 years ago
Sigh. So string predicates fails because we're not dealing with things can nicely turn into boost objs.
So instead did:
private:
const bool starts_with(const std::string_view str, const std::string_view sub) const {
return (str.compare(0, sub.size(), sub)) == 0;
}
const bool ends_with(const std::string_view str, const std::string_view sub) const {
return (str.compare(str.size() - sub.size(), sub.size(), sub)) == 0;
}
Which looks like it's kinda working, well at least now to the next issue. Turns out std::string_view objects don't have a .to_string() method like boost::string_view does. And looking at the make output we seem to use that a lot.
/ceph/src/rgw/rgw_op.cc: In member function ‘boost::optional<std::pair<std::__cxx11::basic_string<char>, rgw_obj_key> > RGWBulkUploadOp::parse_path(std::string_view)’: /ceph/src/rgw/rgw_op.cc:6711:41: error: ‘const class std::basic_string_view<char>’ has no member named ‘to_string’ return std::make_pair(bucket_name.to_string(), ^~~~~~~~~ /ceph/src/rgw/rgw_op.cc:6712:50: error: ‘const class std::basic_string_view<char>’ has no member named ‘to_string’ rgw_obj_key(obj_name.to_string())); ^~~~~~~~~ /ceph/src/rgw/rgw_op.cc:6716:52: error: ‘class std::basic_string_view<char>’ has no member named ‘to_string’ return std::make_pair(path.substr(start_pos).to_string(), ^~~~~~~~~ /ceph/src/rgw/rgw_op.cc: In member function ‘virtual void RGWBulkUploadOp::execute()’: /ceph/src/rgw/rgw_op.cc:7191:66: error: ‘using string_view = class std::basic_string_view<char> {aka class std::basic_string_view<char>}’ has no member named ‘to_string’ file_prefix + header->get_filename().to_string(); ^~~~~~~~~ /ceph/src/rgw/rgw_op.cc:7201:52: error: ‘using string_view = class std::basic_string_view<char> {aka class std::basic_string_view<char>}’ has no member named ‘to_string’ failures.emplace_back(op_ret, filename.to_string()); ^~~~~~~~~ /ceph/src/rgw/rgw_op.cc:7211:51: error: ‘using string_view = class std::basic_string_view<char> {aka class std::basic_string_view<char>}’ has no member named ‘to_string’ failures.emplace_back(op_ret, dirname.to_string()); ^~~~~~~~~ ...
I can go fix up each call, or maybe subclass std::basic_string_view and add a to_string() method ourselves, which would be less code.
If I go that route, then I guess I could also move starts_with and ends_with into it too. And as C++ grows the extra methods (c++20 for {starts|ends}_with) we can just drop them and hopefully eventually from our version of the class.
Updated by Abhinav Singh about 4 years ago
Is this issue free I would like to contribute
Updated by Yuval Lifshitz almost 4 years ago
- Status changed from New to Fix Under Review
- Pull request ID set to 35208
Updated by Casey Bodley almost 4 years ago
- Status changed from Fix Under Review to Resolved