[CXX-1645] Consider deprecating or removing string::view_or_value Created: 06/Sep/18  Updated: 27/Oct/23  Resolved: 05/Oct/18

Status: Closed
Project: C++ Driver
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Minor - P4
Reporter: Kevin Albertson Assignee: Unassigned
Resolution: Works as Designed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to CXX-1827 Reduce friction between view and valu... Backlog

 Description   

In favor of using string_view for all public API.



 Comments   
Comment by Kevin Albertson [ 05/Oct/18 ]

True. mongocxx::options::find::comment doesn't keep a copy of the passed string. It copies it later, when constructing the bson_t to pass to libmongoc: https://github.com/mongodb/mongo-cxx-driver/blob/master/src/mongocxx/collection.cpp#L319

I'm convinced there's no great way to use the current abstractions view/value/view_or_value to come up with a better solution here. I'm closing this, but feel free to re-open if anyone objects.

However, I'd like to discuss opinions of view_or_value expressed in CXX-1633. Before we set ABI, if it's something we end up wanting to change, we should do so now. I'll move this conversation to email.

Comment by Andrew Morrow (Inactive) [ 05/Oct/18 ]

jesse - That isn't true though. See my example above with `mongocxx::options::find::comment`. It definitely doesn't pass its argument to the C driver.

Comment by A. Jesse Jiryu Davis [ 04/Oct/18 ]

Aside from deprecated options::index functions (see CXX-1663), all the functions that take string::view_or_value immediately pass the string to the C Driver, which immediately copies the contents. So we could assume all input strings are const temporaries, delete string::view_or_value, and save ourselves some idiosyncrasy compared to C++ std lib.

Comment by Andrew Morrow (Inactive) [ 28/Sep/18 ]

Correct. The idea was to avoid copies in cases where they could be, but extend lifetimes in cases where they needed to be. In other words, it should be legal to write the following:

mongocxx::options::find_options;
mongocxx::stdx::string_view some_comment = ...
find_options.comment(some_comment);
some_collection.find(some_query, find_options);

Here, no copies are needed, just one move of a stdx::string_view.

If the comment is a temporary, then we need to move it into the value slot:

mongocxx::options::find_options;
find_options.comment(make_me_a_temp_comment());
some_collection.find(some_query);

Comment by Kevin Albertson [ 28/Sep/18 ]

Right, that problem applies whenever we're storing a view to the temporary. What I meant was in cases we don't need to store any view/value. E.g. collection::distinct uses the passed string::view_or_value immediately and does not need to store it for use later. Many other functions taking a string::view_or_value do something similar: the string gets passed to a libmongoc function (which copies) and does not store it elsewhere. But many do keep the view_or_value for later use:

explicit hint(bsoncxx::string::view_or_value index);
change_stream& change_stream::full_document(bsoncxx::string::view_or_value full_doc);
find& find::comment(bsoncxx::string::view_or_value comment);
void index::wiredtiger_storage_options::config_string(bsoncxx::string::view_or_value config_string);
index& index::default_language(bsoncxx::string::view_or_value default_language);
index& index::language_override(bsoncxx::string::view_or_value language_override);
index& index::name(bsoncxx::string::view_or_value name);
ssl& ssl::ca_dir(bsoncxx::string::view_or_value ca_dir);
ssl& ssl::ca_file(bsoncxx::string::view_or_value ca_file);
ssl& ssl::crl_file(bsoncxx::string::view_or_value crl_file);
ssl& ssl::pem_file(bsoncxx::string::view_or_value pem_file);
ssl& ssl::pem_password(bsoncxx::string::view_or_value pem_password);

If we changed these to take string_view instead, they'd need to copy it.

Comment by Andrew Morrow (Inactive) [ 17/Sep/18 ]

I should have been a little clearer in my example, but the same problems exist for both string}}s and {{document}}s. They both have view and value types, there are expressions that produce value types, and temporaries can get bound to the {{view types.

Comment by Kevin Albertson [ 17/Sep/18 ]

Good point. This is for string::view_or_value, not document::view_or_value. We could enforce that if we want to keep a reference to the string after the call, we keep a copy.

Comment by Andrew Morrow (Inactive) [ 11/Sep/18 ]

You are going to find that this is hard to do.

The challenge is code like the following:

auto coll = ...
auto options = mongocxx::options::update{};
options.array_filters( some_expression_returning_a_temporary_document::value );
coll.update(..., options);

Because the temporary document::value} expires at the end of the expression, you have captured a document::view to free'd memory, which you then use in the call to coll.update. This sort of bug was very common in the early development of the driver, and due to lack of agreement on other ways forward, view_or_value was the remedy of choice.

Generated at Wed Feb 07 22:03:26 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.