Project

General

Profile

Actions

Bug #39619

closed

cleanup: replace boost::string_ref/view with c++17 std::string_view

Added by Casey Bodley almost 5 years ago. Updated almost 4 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Target version:
-
% Done:

0%

Source:
Tags:
easy first bug
Backport:
Regression:
No
Severity:
3 - minor
Reviewed:
Affected Versions:
ceph-qa-suite:
Pull request ID:
Crash signature (v1):
Crash signature (v2):
Actions #1

Updated by Matthew Oliver over 4 years ago

Seems pretty straight forward. I'll give this a go.. famous. last. words :P

Actions #2

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.

Actions #3

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.

Actions #4

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.

Actions #5

Updated by Abhinav Singh about 4 years ago

Is this issue free I would like to contribute

Actions #6

Updated by Yuval Lifshitz almost 4 years ago

  • Status changed from New to Fix Under Review
  • Pull request ID set to 35208
Actions #7

Updated by Casey Bodley almost 4 years ago

  • Status changed from Fix Under Review to Resolved
Actions

Also available in: Atom PDF