Add 'operator==' for 'basic_collection_iterator'

[Alexey Denisov via llvm-commits]

Oh, my bad. This is so obvious...
Thank you for your help and for your patience.

Here is a new version, though I didn’t manage to write test for ’std::next(begin) == begin’ because of non-reentrant nature of YAML iterator. If I understand it correctly to make it working I do need to implement shallow copy for iterator, hence to implement shallow copy of SequenceNode and Node, which I considered an overkill for this task.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: basic_collection_iterator_operator_equals.patch
Type: application/octet-stream
Size: 3450 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151217/616271a6/attachment.obj>
-------------- next part --------------


I would appreciate any other feedback.
--
AlexDenisov
Software Engineer, http://lowlevelbits.org

> On 17 Dec 2015, at 18:49, Jordan Rose <jordan_rose at apple.com> wrote:
> 
> Your test sequences only have one element. If they had two elements, then "std::next(begin)" would have the same 'Base' as "begin", but a different 'CurrentEntry'.
> 
> Jordan
> 
>> On Dec 17, 2015, at 2:04 , Alexey Denisov <1101.debian at gmail.com> wrote:
>> 
>>> "std::next(begin) == begin" should not succeed either.
>> 
>> Indeed, it doesn’t: operator== returns false, since ’std::next(begin)' equals to '++begin’ and equals to ‘end’ in the particular test.
>> 
>>> the existing "end == end" test will probably fail as well.
>> 
>> As far as I see it behaves correctly, since I took ‘operator!=’s behaviour as a point of truth, i.e.: ‘end != end -> false’.
>> 
>> 
>> Currently I’m confused a bit. Let me describe the flow to explain source of confusion:
>> 
>> When pointed me to the lack of tests I did the following:
>> 
>> 1. Rolled back my changes
>> 2. Implemented all the tests presented in the most recent patch
>> 3. Implemented ‘operator==‘ to return false -> some ‘==‘-related tests failed
>> 4. Implemented ‘operator==‘ as negation of ‘operator!=‘ (i.e.: ‘!(*this != Other)') -> tests passed
>> 5. Moved inverted implementation of ‘operator!=‘ into ‘operator==‘ -> tests passed
>> 6. Implemented ‘operator!=‘ as negation of ‘operator==‘ (i.e.: ‘!(*this == Other)’) -> tests passed
>> 
>> So I still didn’t get what is wrong with the implementation.
>> 
>> I would appreciate if you can shed more light on the problem.
>> --
>> AlexDenisov
>> Software Engineer, http://lowlevelbits.org
>> 
>>> On 17 Dec 2015, at 03:46, Jordan Rose <jordan_rose at apple.com> wrote:
>>> 
>>> Well, the YAML sequences aren't really re-entrant, but "std::next(begin) == begin" should not succeed either. And then once that's fixed, the existing "end == end" test will probably fail as well.
>>> 
>>> Besides that, though, the tests look good! Thanks for doing this.
>>> Jordan
>>> 
>>>> On Dec 16, 2015, at 14:03 , Alexey Denisov <1101.debian at gmail.com> wrote:
>>>> 
>>>> Hmm, I did add couple of tests, but can’t catch wrong behaviour.
>>>> What am I missing here?..
>>>> 
>>>> <basic_collection_iterator_operator_equals.patch>
>>>> 
>>>> --
>>>> AlexDenisov
>>>> Software Engineer, http://lowlevelbits.org
>>>> 
>>>>> On 15 Dec 2015, at 23:59, Jordan Rose <jordan_rose at apple.com> wrote:
>>>>> 
>>>>> The logic here is wrong:
>>>>> 
>>>>>> +  bool operator==(const basic_collection_iterator &Other) const {
>>>>>> +    if (Base == Other.Base)
>>>>>>    return true;
>>>>> 
>>>>> This test is backwards (early exit on !=, not ==), and the null check below is now incorrect.
>>>>> 
>>>>> I generally do support being consistent about operators, so I'm in favor of the patch, but please write some tests as well. unittests/Support/YAMLParserTest.cpp is probably a good place.
>>>>> 
>>>>> Jordan
>>>>> 
>>>>> 
>>>>>> On Dec 15, 2015, at 13:14 , Alexey Denisov <1101.debian at gmail.com> wrote:
>>>>>> 
>>>>>> Patch adds ‘operator==‘ implementation for ‘basic_collection_iterator’
>>>>>> 
>>>>>> Motivation:
>>>>>> 
>>>>>> Swift compiler uses workaround since 'operator==‘ is not implemented.
>>>>>> 
>>>>>> Driver.cpp:220
>>>>>> 
>>>>>> if (!(seqI != seqE))
>>>>>> return true;
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> <basic_collection_iterator_operator_equals.patch>
>>>>>> 
>>>>>> --
>>>>>> AlexDenisov
>>>>>> Software Engineer, http://lowlevelbits.org
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 496 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151217/616271a6/attachment.sig>