Skip to content

feat: Allow decoding for types without default constructor (V2) - #1488

Draft
SGSSGene wants to merge 2 commits into
jbeder:masterfrom
SGSSGene:feat/not-default-decoder-expected
Draft

feat: Allow decoding for types without default constructor (V2)#1488
SGSSGene wants to merge 2 commits into
jbeder:masterfrom
SGSSGene:feat/not-default-decoder-expected

Conversation

@SGSSGene

@SGSSGene SGSSGene commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

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>::decode in #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::expected type.

Assume we have some type without default constructor:

class Vec3 {
  double x, y, z;
public:
  Vec3(double x, double y, double z} : x{x}, y{y}, z{z} {}
};

you could write

namespace YAML {
template<>
struct convert<Vec3> {
  static auto decode(const Node& node) -> expected<Vec3> {
    if(!node.IsSequence() || node.size() != 3) {
      return unexpected{};
    }
    return expected<Vec3> {
        node[0].as<double>(),
        node[1].as<double>(),
        node[2].as<double>()
    );
  }
};
}

@SGSSGene
SGSSGene marked this pull request as draft September 2, 2026 08:28
@SGSSGene
SGSSGene force-pushed the feat/not-default-decoder-expected branch from f1bd548 to 7456f60 Compare September 2, 2026 08:31
@SGSSGene SGSSGene changed the title Feat/not default decoder expected feat: Allow decoding for types without default constructor (V2) Sep 2, 2026
@alex-thiessen-for-siemens

Copy link
Copy Markdown

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?

@SGSSGene

SGSSGene commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

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!

@alex-thiessen-for-siemens alex-thiessen-for-siemens left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Takes me some time to get used to GitHub's review process, sorry.

@alex-thiessen-for-siemens alex-thiessen-for-siemens left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • 9f0b75c verifies that expected.h can be included directly.
  • 68303d5 preserves the existing null-node conversions.
  • 7e0a63b covers move-only expected<T> and Node::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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The convert<T>::decode() and Node::as<T>() signature inconsistency.

2 participants