Defect in c-tor

[KristianIvarsson]

Description

There seem to be some flaw in move c-tor that makes the type not become as explected

Maybe a simple std::variant and using default implementation would be more simple

Reproduction steps

   const fkyaml::node a = fkyaml::node::mapping();
   const fkyaml::node b{ fkyaml::node::mapping()};
   assert( a.get_type() == b.get_type());

Expected vs. actual results

Expected

assert( b.get_type() == fkyaml::node::MAPPING);

Actual

assert( b.get_type() == fkyaml::node::SEQUENCE);

Minimal code example

Error messages

Compiler and operating system

g++13 Ubuntu

Library version

g++13

Validation

• [ ] The bug also occurs if the latest version from the develop branch is used.
• [ ] I can successfully compile and run the unit tests.

[sndth]

Hi! Thank you for reporting! This (probably) isn't a bug in the library, but a result of how different compilers handle move semantics:

const fkyaml::node b{ fkyaml::node::mapping()};

GCC and Clang perform a move from the temporary returned by mapping(), which clears internal state (like m_attrs) in the move constructor. MSVC, on the other hand, aggressively applies copy elision, so no move happens - the temporary is constructed directly into b, preserving the type.

That's why get_type() returns different results on each compiler. Best fix is avoid brace initialization with temporaries. Use assignment instead:

const auto b = fkyaml::node::mapping(); // Consistent across all compilers

Let me know if you'd prefer a patch that preserves type info after move!

[KristianIvarsson]

Yes, I guess RVO is applied in the former construct, but the observable behaviour should be the same in the latter case

I'm not sure if users always have the possibility to avoid brace initialization (e.g. a c-tor for a const member or such)

Let me know if you'd prefer a patch that preserves type info after move!

I'm not 100% sure what you mean by preserve, but isn't that what should happen? Shouldn't every property of rhs be moved/copied into lhs?

[KristianIvarsson]

I looked a bit more into this @sndth

When using brace-initialization, the basic_node(initializer_list_t init) c-tor is (naturally) invoked as well. Removing that c-tor make things work as expected. Maybe there's a flaw in that one ?

The node_ref->is_sequence() looks a bit fishy in

        bool is_mapping =
            std::all_of(init.begin(), init.end(), [](const detail::node_ref_storage<basic_node>& node_ref) {
                // Do not use is_sequence_impl() since node_ref may be an anchor or alias.
                return node_ref->is_sequence() && node_ref->size() == 2;
            });

Is the initializer-list c-tor really needed ?

Maybe it's too much work (refactoring), but I'd go with some standard data structure for all this and then not have any explicit c-tors or assignment operators what so ever

[sndth]

Hi! Sorry for the long wait! 😅 I've prepared a fix for GCC so that braced-init behaves like MSVC for empty collections.

With this patch, both:

const node a = node::mapping();
const node b{ node::mapping() };

and

const node a = node::sequence();
const node b{ node::sequence() };

now produce the same result on GCC and MSVC.

The special-case unwrap is applied only when the initializer list contains exactly one element and that element is an empty mapping or empty sequence with no extra properties (tags, anchors, etc.). This resolves the original { mapping() }/{ sequence() } mismatch without breaking any of the existing initializer list semantics.

It doesn't change the behavior for scalars - for example:

const node a = node::boolean_type(true);  // Scalar
const node b{ node::boolean_type(true) }; // Still a 1-element sequence

MSVC sometimes elides here and makes it "look" like a scalar move, but that's not portable and is out of scope for this fix. If needed, we can discuss an opt-in to unwrap scalars as well, but by default braces are meant for collections and parentheses for single values.

Could you try this branch on your side and see if it solves your original case?

[KristianIvarsson]

Okey, cool

I will try it out someday (hopefully soon)

Using braces for initialization is called Uniform Initialization and is not only meant for collections (according to the standard)