Add 'operator==' for 'basic_collection_iterator'

Alexey Denisov via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 18 05:49:21 PST 2015 

> Ha, I was still thinking something simpler.

I came up with the same solution when I woke up next morning :-D

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

I have faced an issue with such tests. std::next in the case of SequenceNode mutates value in-place, so that iterators are equal.
But it could be tested using some other BaseT, not a SequenceNode. I also realised it afterwards.

> 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.

I will look at this issue as well.
--
AlexDenisov
Software Engineer, http://lowlevelbits.org

> On 18 Dec 2015, at 04:09, Jordan Rose <jordan_rose at apple.com> wrote:
> 
> 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 --------------
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/20151218/ee1dd56d/attachment.sig>

More information about the llvm-commits mailing list