[SERVER-26542] Migrate to std::optional Created: 09/Oct/16  Updated: 08/Jan/24

Status: Backlog
Project: Core Server
Component/s: Portability
Affects Version/s: None
Fix Version/s: None

Type: New Feature Priority: Major - P3
Reporter: Andrew Morrow (Inactive) Assignee: Backlog - Service Architecture
Resolution: Unresolved Votes: 2
Labels: sa-remove-fv-backlog-22
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to SERVER-68121 clang-tool boost::optional -> std::op... Backlog
related to SERVER-68246 change calls to `boost::optional` mem... Closed
Assigned Teams:
Service Arch
Backwards Compatibility: Fully Compatible
Participants:

 Description   

Originally this ticket was about using std::optional where possible, and boost::optional elsewhere. The idea of this polyfill was we could avoid including the boost headers in many places if the system provides a std::optional (from std::experimental or similar). After some investigation, the ticket is now about replacing our use of boost::optional with std::optional entirely.



 Comments   
Comment by Billy Donahue [ 18/Nov/20 ]

There are significantly more significant differences between boost::optional and std::optional than the previously mentioned get_ptr and boost::none.

The hardest one is that boost::optional_io.hpp provides an operator<< for optional<T>. This is not sfinae-friendly and will static-assert at you if you try to call it. A boost::optional<T> always appears to be streamable whether T is really streamable or not. This ALREADY screws up detection traits for streamability in frameworks like fmtlib, logv2, StatusWith, or unittest asserts, which all have to handle boost::optional as a special case.

https://github.com/boostorg/optional/blob/develop/include/boost/optional/optional.hpp#L1594
In those places we are trying to do a structurally recursive traits check for a property like streamability and they need to each be rewritten to correctly identify and define a convention for std::optional streaming (easiest is to just reuse the boost/optional_io.hpp output format). There's a generic pluggable-policy-based solution to problems of this type that I think would make an interesting project for reuse across our stringification, logging, and BSON libraries and greatly offload their complexity and redundancy. Kind of a SAX-like descent protocol. Then we can stop reinventing this wheel.

I've prototyped making the custom changes to each of the places where we do structurally recursive visitation for streamability here: https://github.com/mongodb/mongo/compare/master...BillyDonahue:boost_optional_io_wean?expand=1
This gets us completely out of the optional_io.hpp business.

The most common operations on an optional are has_value() and value(). These would have to be identified and renamed from boost's is_initialized() and get(), respectively. Here's a tool to do that.
https://github.com/BillyDonahue/llvm-project/compare/master...BillyDonahue:boost-optional-migration?expand=1
We can also change these to operator* and operator-> and explicit operator bool expressions which would yield code that works in EITHER boost::optional or std::optional.

There are a few uses of boost::optional<T&>, which isn't supported in std::optional. All uses in our codebase seem to be unnecessary cleverness that can be trivially replaced with T* without change of semantics.

There are conditional constructors in boost::optional that std::optional doesn't have. They take a (bool,T&&) kind of signature, constructing an empty optional if the bool is false. These are easily rewritten and there aren't that many of them. Maybe 10 or 20.

std::optional is convertible from values that are convertible. boost::optional is not. This could yield sfinae (or similar) ambiguities that must be resolved in a transition.

Boost::optional has some monadic operations like value_or which std::optional has. Others like map and flat_map are missing from std::optional but we have only a couple of uses that can spell it out with explicit lambdas.

There's a difference between boost::optional::value_or and boost::optional::get_value_or, which is deprecated because it returns a reference that's often dangling. We should scrub our code of get_value_or anyway, but this migration would make the scrub mandatory.

The get_ptr problem isn't so bad. All call sites in practice can be replaced with a ternary (opt?&*opt:nullptr). At most calls of get_ptr (there are not that many), we already tested the opt's bool value and can just replace opt.get_ptr() with &*opt.

More trivial things:

Obviously we'd have to rewrite the #include lines.

Replace all the declarations of:
boost::optional to std::optional
boost::none to std::nullopt
boost::none_t to std::nullopt_t
boost::in_place to std::in_place
boost::in_place_t to std::in_place_t

IDL would need some minor surgery to make it emit std::optional compatible code. That was pretty easy to do.

Comment by Andrew Morrow (Inactive) [ 29/Jun/19 ]

I propose we remove this from the C++17 epic, retitle it to "Migrate to std::optional", and take it out of the sprint. The re-titled ticket can live on the backlog until we decide that a sufficient motivation has emerged to make the switch over to std::optional.

Comment by Mira Carey [ 25/Jun/19 ]

deduction guides (like those on std::optional) might also be nice.

I think I agree with acm. I'm in the camp of: "using std types is a nice, but this looks like a wide change that delivers relatively little value". I'm not too worried that we're going to run into bad implementations of std::optional (like we have for variant), it's just a lot of motion for a world that's basically where we are today.

In terms of future integration with the language, I'm not aware of anything that's currently returning an optional in the standard lib. The only area I see work kicking around is in support for <=> for optional. But I'd assume that's something that would make it's way to boost (with the same semantics) if/when that lands

 

Comment by Andrew Morrow (Inactive) [ 25/Jun/19 ]

jesse - To be honest that doesn't sound that bad since most would be an easy search and replace. If we are going to do it I think we probably wouldn't even polyfill, just cut straight over to std::optional. I guess the other question is what if anything we gain by switching over. I'd expect redbeard0531, adam.martin@mongodb.com and mira.carey@mongodb.com may have opinions. My feeling is that we are getting along just fine with boost::optional. On the other hand if something coming in C++20 makes deeper connection with std::optional we could end up with an annoying impedance mismatch if we were still on boost::optional when we got there.

Comment by A. Jesse Jiryu Davis [ 24/Jun/19 ]

Bartek has a list of boost/std::optional differences, I think these two matter to us:

  • boost::optional has get_ptr() but std::optional doesn't. We use get_ptr in about 39 places which would have to be replaced.
  • A disengaged boost::optional equals boost::none, but a disengaged std::optional equals std::nullopt. We use boost::none in about 1772 places. We'd have to typedef our own stdx::nullopt as either std::nullopt or boost::none depending on which is available.
Generated at Thu Feb 08 04:12:27 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.