std::experimental::optional #475
Conversation
aldanor
force-pushed
the
feature/optional
branch
2 times, most recently
from
c38a249
to
5b80f49
Compare
|
Perhaps also let it support std::optional (instead std::experimental::optional) if available, so that it won't need change under a C++17 compiler without std::experimental::optional? In fact, the example in the |
|
Looks useful! All agreed about the std::experimental prefix -- it should work out of the box on a C++17 capable compiler without experimental namespaces. PS: do you also have plans for std::variant? |
|
@jagerman's suggestion looks good, will add that. It's going to stay untested for a while though :)
|
|
One detail: I guess Thus, I think the type caster should be defined separately for both (although it would be identical). |
|
Hmm-- if |
|
Not sure, ask GCC guys :) But they seem to be actively maintaining both. I guess so as not to break downstream code that uses the experimental version if there are radical changes in the |
|
Aha, I see now. I think this could be done similarly to other parts of |
|
Ok, this should do it. |
| template<typename T> struct type_caster<std::optional<T>> | ||
| : public optional_caster<std::optional<T>> {}; | ||
| NAMESPACE_END(detail) | ||
| NAMESPACE_END(pybind11) |
There was a problem hiding this comment.
Very subjective opinion: it may be nice to use the more compact namespace pybind11 { namespace detail { and }} here. Just to improve the signal-to-noise ratio.
There was a problem hiding this comment.
Agreed, too many shouty macros.
There was a problem hiding this comment.
It might be even easier to do all the #includes at the top and make definitions like PYBIND11_HAS_OPTIONAL and PYBIND11_HAS_EXP_OPTIONAL
Then, you could #ifdef those templates without having to open and close namespaces all the time.
There was a problem hiding this comment.
Both is fine to me though, whatever you prefer.
There was a problem hiding this comment.
Yea that could be cleaner. I also missed one case apparenty (due to using elif instead of if). Fix incoming
aldanor
force-pushed
the
feature/optional
branch
3 times, most recently
from
16b1a19
to
451216b
Compare
|
All done, there's now defs in |
aldanor
force-pushed
the
feature/optional
branch
from
451216b
to
25252de
Compare
|
Sorry, one more thing: can you add a line to the changelog and add std::optional to the table of supported types in the documentation? |
|
Ok sure. We can now even say we support some C++17 features 🐼 |
aldanor
force-pushed
the
feature/optional
branch
from
a5ed27d
to
827272d
Compare
|
Added a mention in the docs (unfortunately had to reflow the table), rebased on master. |
|
Great, thank you! |
|
Great work, thank you! Is there any chance you could make it work with VS15 Preview 5 too? (this is the "next" version of VS, currently in Release Candidate status - not VS2015, which doesn't have The coming "VS15" has |
|
If this is important to you, I suggest that you create a PR with a suitable VS2015-compatible detection method. |
|
I went back to VS2015 for now. I will revisit this once the final version of the next VS is out. (should be quite soon :-) ) |
I'm not sure this would be accepted as PR given it's a TS feature of C++ as of C++14, but we use this internally a lot and find it very useful, so I figured I'd share :)
That being said, it's just a single tiny template in
stl.hconditional on C++ standard library feature being present, and it fits pybind11 pretty well, don't see any harm in merging it in.// optional has been finally accepted into C++ standard earlier this year, so it's not going away and is going to become std::optional in C++17 with the same semantics; std::experimental::optional has been around for quite a while now, e.g. in gcc since 4.9.