Adding support for handling YAML Merge Key (#41) - proper keys comparison for merging #1279
Conversation
Support for YAML Merge keys ( <<: [*dict1, *dict2] ) is added. The merge key is a specific scalar with value << (and tag !!merge) that implies that during node construction, the map (or sequence of maps) are merged into the current map. The priority rules are that each key from maps within the value associated with << are added iff the key is not yet present in the current map (and first map gets higher priority). Test cases have been added accordingly.
…the node) The problem is that const_map_to.get(*j->first) compares only the shared_ptr's, while we need to compare the key itself. v2: remove const iterator v3: fix use-after-free due to const reference
Consider the following YAML:
trait1: &t1
foo: 1
trait2: &t2
foo: 2
merged:
<<: *t1
<<: *t2
yq reports:
$ yq .merged.foo < /tmp/yaml
2
while the order that yaml-cpp returns is different, since it will
firstly handle 1, and will not replace it with 2:
$ util/parse < /tmp/yaml
trait1:
? &1 foo
: &2 1
trait2:
foo: 2
merged:
*1 : *2
(Don't mix up "*2" with "2", it is trait1)
src/nodebuilder.cpp
Outdated
| void MergeMapCollection(detail::node& map_to, detail::node& map_from, | ||
| detail::shared_memory_holder& pMemory) { |
There was a problem hiding this comment.
| void MergeMapCollection(detail::node& map_to, detail::node& map_from, | |
| detail::shared_memory_holder& pMemory) { | |
| static void MergeMapCollection(detail::node& map_to, detail::node&& map_from, | |
| detail::shared_memory_holder& pMemory) { |
This function should be static or in an anonymous namespace to not leak any symbols to other translation units.
I also believe this is also a good case for a r-value reference for map_from. So I would use a && to make clear that map_from will be in an unclear state after this call.
There was a problem hiding this comment.
This function should be static or in an anonymous namespace to not leak any symbols to other translation units.
Done
I also believe this is also a good case for a r-value reference for map_from. So I would use a && to make clear that map_from will be in an unclear state after this call.
I'm not sure about this one, since map_from is in valid state upon return from MergeMapCollection, what make you thinks the opposite?
There was a problem hiding this comment.
I don't know what I was thinking last month....It seems fine way it is 😅
| auto& toMerge = *m_mergeDicts.rbegin(); | ||
| /// The elements for merging should be traversed in reverse order to prefer last values. | ||
| for (auto it = toMerge.rbegin(); it != toMerge.rend(); ++it) { | ||
| MergeMapCollection(collection, **it, m_pMemory); |
There was a problem hiding this comment.
If MergeMapCollection is being adjusted as suggested above **it must be changed to std::move(**it)
NOTE: this is based on #1243, but includes a fix for proper merging (by comparing the key itself, instead of shared_ptrs, see the last patch).
Support for YAML Merge keys ( <<: [*dict1, *dict2] ) is added. The merge
key is a specific scalar with value << (and tag !!merge) that implies
that during node construction, the map (or sequence of maps) are merged
into the current map. The priority rules are that each key from maps
within the value associated with << are added iff the key is not yet
present in the current map (and first map gets higher priority). Test
cases have been added accordingly.