Add 'operator==' for 'basic_collection_iterator'

[Jordan Rose via llvm-commits]

Ha, I was still thinking something simpler.

diff --git a/include/llvm/Support/YAMLParser.h b/include/llvm/Support/YAMLParser.h
index b056ab6..d9ac360 100644
--- a/include/llvm/Support/YAMLParser.h
+++ b/include/llvm/Support/YAMLParser.h
@@ -327,10 +327,15 @@ public:
   }
 
   bool operator!=(const basic_collection_iterator &Other) const {
+    return !(*this == Other);
+  }
+
+  bool operator==(const basic_collection_iterator &Other) const {
     if (Base != Other.Base)
+      return false;
+    if (!Base && !Other.Base)
       return true;
-    return (Base && Other.Base) &&
-           Base->CurrentEntry != Other.Base->CurrentEntry;
+    return Base->CurrentEntry == Other.Base->CurrentEntry;
   }
 
   basic_collection_iterator &operator++() {


And then for the unit tests I think we're okay, since we're not actually dereferencing the iterator:

EXPECT_FALSE(Begin == std::next(Begin));
EXPECT_FALSE(std::next(Begin) == Begin);

InputIterators don't actually guarantee that this is valid, so we don't have to implement it, but I think we should do it just so other people don't get bitten. There's not much reason not to.

Jordan

> On Dec 17, 2015, at 12:46, Alexey Denisov <1101.debian at gmail.com> wrote:
> 
> 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.
> 
> <basic_collection_iterator_operator_equals.patch>
> 
> 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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151217/701deccc/attachment.html>