[PATCH] Fix YAML sequences with default entries

Aaron Ballman aaron at aaronballman.com
Thu Aug 15 16:22:31 PDT 2013


Your changes worked great!  I've committed as r188508 with some minor
tweaks to the unit test (I removed some commented out code that was
accidentally left in your patch, and added some more tests to make
sure that elements other than the first in the sequence are still
properly elided).

Thanks!

~Aaron

On Wed, Aug 14, 2013 at 3:42 PM, Nick Kledzik <kledzik at apple.com> wrote:
> [resend with patch attached]
>
>
>
> On Aug 13, 2013, at 6:50 PM, Nick Kledzik <kledzik at apple.com> wrote:
>> Thanks for digging into this and making a patch and test case.  After reviewing it, I think the change is too strong and prevents some valid key/value-seq elides.  For instance, if the map has another key/value pair, it is ok to elide the empty sequence because the other key/value keeps a entry in the top level sequence .
>>
>> Here is a patch that tightens it up a bit and only stops eliding empty sequences if the embedded empty sequence is the first key/value in a map which is itself in a sequence.
>>
>> I also, moved the implementation of the checks out of the header and into YAMLTraits.cpp, so that all clients don’t carry a copy.  And I changed:
>>  EXPECT_EQ(2, Seq2.Tests.size());
>> to:
>>  EXPECT_EQ(2UL, Seq2.Tests.size());
>> to prevent compiler comparison warnings.
>>
>> Please try this out on whatever real world yaml case you were using to verify it fixes your problem.  Thanks.
>>
>> -Nick
>>
>>
>> On Aug 13, 2013, at 4:09 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>> When creating a YAML sequence where an element is a mapping that is
>>> defaulted, the current implementation elides the element entirely,
>>> which causes round-tripping problems.  Since the output drops the
>>> defaulted entries, the input never receives the correct number of
>>> entries in the sequence.
>>>
>>> The attached patch resolves this issue by checking to see whether a
>>> mapping operation is currently processing a sequence before deciding
>>> to elide the mapped element.  Unit tests demonstrating the issue are
>>> included.
>>>
>>> ~Aaron
>>> <yaml.patch>
>
>




More information about the llvm-commits mailing list