feat: Allow decoding for types without default constructor (V2) - #1488
feat: Allow decoding for types without default constructor (V2)#1488SGSSGene wants to merge 2 commits into
Conversation
f1bd548 to
7456f60
Compare
7456f60 to
372f60b
Compare
|
As a user, I like this approach way more than the one before. If the policy is to provide full feature set even to C++11 users, that's OK with me, too. Sol and I have a couple suggestions regarding the code, I guess we can proceed here, then? |
Yes please, I think continuing here is good! |
There was a problem hiding this comment.
Thanks for working on this. I like the expected-returning conversion approach,
but I found three correctness regressions that should be addressed before
merging, plus one broken documentation example.
I verified the implementation findings with intentionally red, test-only
commits based directly on this PR's head:
https://github.com/alex-thiessen-for-siemens/yaml-cpp/tree/review/1488-regression-tests
9f0b75cverifies thatexpected.hcan be included directly.68303d5preserves the existing null-node conversions.7e0a63bcovers move-onlyexpected<T>andNode::as<T>()conversions.
1. expected.h is not self-contained
include/yaml-cpp/expected.h uses std::array, std::forward, std::move,
and placement new without directly including the headers that declare them.
A translation unit that includes only this public header fails to compile and
currently depends on transitive includes from other yaml-cpp headers.
Please add the required direct includes (<array>, <new>, and <utility>)
and retain a standalone-header compilation test.
2. Null nodes bypass existing converters
The new dispatch paths in include/yaml-cpp/node/impl.h return
unexpected{} for node.IsNull() before invoking the requested converter.
That changes existing behavior for converters that intentionally accept null:
convert<Node>::decode copies null nodes, and convert<_Null>::decode
accepts them.
As a result, both of these now throw TypedBadConversion:
YAML::Node(YAML::NodeType::Null).as<YAML::Node>();
YAML::Node(YAML::NodeType::Null).as<YAML::_Null>();Please preserve the legacy two-argument converter behavior by letting the
converter decide whether null is valid. If expected-returning converters need
a distinct null policy, it should not change existing converters.
3. Move-only decoded types cannot pass through as<T>()
The expected move constructor calls emplace(*o.ptr), which
copy-constructs the stored value. Both as_if return paths likewise return
*t as an lvalue and attempt another copy. Consequently, a
non-default-constructible, move-only type still cannot use the new API.
Please move the value in the expected move constructor and from both
as_if paths:
emplace(std::move(*o.ptr));
return std::move(*t);The tutorial should also state the exact construction requirements for decoded
types rather than only saying that they need not be default-constructible.
4. The documented decoder declaration is invalid C++
docs/Tutorial.md declares:
static bool decode(const Node& node) -> expected<Vec3> {A trailing return type requires auto; bool also conflicts with the stated
expected<Vec3> return type. Please use either:
static auto decode(const Node& node) -> expected<Vec3> {or:
static expected<Vec3> decode(const Node& node) {Non-blocking API design suggestion
When __cpp_lib_expected indicates standard-library support, it may be worth
using std::expected rather than maintaining the storage implementation
locally. This cannot be a direct one-parameter alias: std::expected requires
both T and an error type, whereas this API exposes YAML::expected<T> with
an empty YAML::unexpected sentinel. A standard-backed path would therefore
need a fixed error type and compatible construction API, or a small adapter
that preserves the current interface.
This is an alternative to #1479, #1010 and #1087. Fixes #506, #973 and #993.
As @alex-thiessen-for-siemens noted the suggested signature of
convert<T>::decodein #1479 is not great.I dislike having yaml-cpp in a state where it provides different functionality depending on the C++ version used.
So, I applied some c++ magic (SFINAE) and implemented a minimalistic replacement for
std::expected.I think it unites many desired properties: It is c++11 compatible, it has a nice signature for
decode.It leaves the original API untouched, but extends it nicely.
Disadvantage is, now we have a custom
YAML::expectedtype.Assume we have some type without default constructor:
you could write