[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: |
|
||||||||
| 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 | ||||||||||||
| 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:
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:
| ||||||||||||
| 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:
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:
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. |