fktn-k/fkYAML

Flow-style mapping values cause incorrect parse results

Closed this issue · 3 comments

stephenwhittle commented

Description

Flow-style mappings as values of an outer mapping cause subsequent entries in the outer mapping to be placed in the wrong location in the parsed tree.

Reproduction steps

The following minimal YAML file demonstrates the issue:

"DIEP0":
  DIEPCTL[0]: { name: CTL }
  description: Device IN endpoint 0

Expected vs. actual results

When using fkyaml::node::deserialize on the above fragment, the description element is placed at the top of the parsed tree as a sibling of DIEP0, rather than as a child of it (and sibling to the DIEPTCL[0] node).

Removing the call to m_node_stack.pop_back() at deserializer.hpp:283 appears to avoid the incorrect traversal by deferring to the traversal code at deserializer.hpp:369-375 in my testing, but I am uncertain if that is the appropriate, or sole fix, required.

Minimal code example

No response

Error messages

No response

Compiler and operating system

VS 2022 (both clang-cl and msvc compilers), Win 10

Library version

8bdce5e

Validation

fktn-k commented

@stephenwhittle
Thanks for sharing the issue as well as your suggestion to fix it.
I'm not sure that the deferring doesn't break the history of indentations.

Removing the call to m_node_stack.pop_back() at deserializer.hpp:283 appears to avoid the incorrect traversal by deferring to the traversal code at deserializer.hpp:369-375 in my testing

Some additional changes related to this untouched topic (#175), may be required.

Do you have time to check whether or not your suggestion will do by running the unit tests?
If not, I can deal with it after #281 is solved which I guess won't affect this issue.

stephenwhittle commented

I can confirm that the unit tests pass successfully on both MSVC and clang-cl with the suggested change, but happy for you to consider alternative solutions if you think they may be more robust for scenarios not currently covered by the test suite.

fktn-k commented

@stephenwhittle
In my opinion, it's suffice to say that the change has fixed the issue, at least for now.
If you can afford to add that test case, could you please create a PR so I can incorporate it into the project as your modification?

  • 👍1