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