<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">Ha, I was still thinking something simpler.</div><div class=""><br class=""></div><blockquote style="margin: 0 0 0 40px; border: none; padding: 0px;" class=""><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(148, 58, 32); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""><b class="">diff --git a/include/llvm/Support/YAMLParser.h b/include/llvm/Support/YAMLParser.h</b></span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(148, 58, 32); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""><b class="">index b056ab6..d9ac360 100644</b></span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(148, 58, 32); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""><b class="">--- a/include/llvm/Support/YAMLParser.h</b></span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(148, 58, 32); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""><b class="">+++ b/include/llvm/Support/YAMLParser.h</b></span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(1, 163, 175); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class="">@@ -327,10 +327,15 @@</span><span style="font-variant-ligatures: no-common-ligatures; color: #4c2f2d" class=""> public:</span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(76, 47, 45); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> }</span></div></div><div class=""><p style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(76, 47, 45); background-color: rgb(223, 219, 196); min-height: 13px;" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> </span></p></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(76, 47, 45); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> bool operator!=(const basic_collection_iterator &Other) const {</span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(0, 165, 0); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class="">+ return !(*this == Other);</span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(0, 165, 0); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class="">+ }</span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(0, 165, 0); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class="">+</span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(0, 165, 0); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class="">+ bool operator==(const basic_collection_iterator &Other) const {</span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(76, 47, 45); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> if (Base != Other.Base)</span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(0, 165, 0); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class="">+ return false;</span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(0, 165, 0); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class="">+ if (!Base && !Other.Base)</span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(76, 47, 45); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> return true;</span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(177, 37, 18); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class="">- return (Base && Other.Base) &&</span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(177, 37, 18); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class="">- Base->CurrentEntry != Other.Base->CurrentEntry;</span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(0, 165, 0); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class="">+ return Base->CurrentEntry == Other.Base->CurrentEntry;</span></div></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(76, 47, 45); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> }</span></div></div><div class=""><p style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(76, 47, 45); background-color: rgb(223, 219, 196); min-height: 13px;" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> </span></p></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(76, 47, 45); background-color: rgb(223, 219, 196);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> basic_collection_iterator &operator++() {</span></div></div></blockquote><div class=""><span style="font-variant-ligatures: no-common-ligatures" class=""><br class=""></span></div><div class=""><br class=""></div><div class="">And then for the unit tests I think we're okay, since we're not actually dereferencing the iterator:</div><div class=""><br class=""></div><blockquote style="margin: 0 0 0 40px; border: none; padding: 0px;" class=""><div class=""><div class="">EXPECT_FALSE(Begin == std::next(Begin));</div></div><div class=""><div class="">EXPECT_FALSE(std::next(Begin) == Begin);</div></div></blockquote><div class=""><br class=""></div><div class="">InputIterators don't actually guarantee that this is valid, so we don't <i class="">have</i> 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.</div><div class=""><br class=""></div><div class="">Jordan</div><br class=""><div><blockquote type="cite" class=""><div class="">On Dec 17, 2015, at 12:46, Alexey Denisov <<a href="mailto:1101.debian@gmail.com" class="">1101.debian@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">Oh, my bad. This is so obvious...<br class="">Thank you for your help and for your patience.<br class=""><br class="">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.<br class=""><br class=""><span id="cid:A0BC891B-EFB2-4612-8495-41092CB7612F@apple.com"><basic_collection_iterator_operator_equals.patch></span><br class=""><br class="">I would appreciate any other feedback.<br class="">--<br class="">AlexDenisov<br class="">Software Engineer, <a href="http://lowlevelbits.org" class="">http://lowlevelbits.org</a><br class=""><br class=""><blockquote type="cite" class="">On 17 Dec 2015, at 18:49, Jordan Rose <<a href="mailto:jordan_rose@apple.com" class="">jordan_rose@apple.com</a>> wrote:<br class=""><br class="">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'.<br class=""><br class="">Jordan<br class=""><br class=""><blockquote type="cite" class="">On Dec 17, 2015, at 2:04 , Alexey Denisov <<a href="mailto:1101.debian@gmail.com" class="">1101.debian@gmail.com</a>> wrote:<br class=""><br class=""><blockquote type="cite" class="">"std::next(begin) == begin" should not succeed either.<br class=""></blockquote><br class="">Indeed, it doesn’t: operator== returns false, since ’std::next(begin)' equals to '++begin’ and equals to ‘end’ in the particular test.<br class=""><br class=""><blockquote type="cite" class="">the existing "end == end" test will probably fail as well.<br class=""></blockquote><br class="">As far as I see it behaves correctly, since I took ‘operator!=’s behaviour as a point of truth, i.e.: ‘end != end -> false’.<br class=""><br class=""><br class="">Currently I’m confused a bit. Let me describe the flow to explain source of confusion:<br class=""><br class="">When pointed me to the lack of tests I did the following:<br class=""><br class="">1. Rolled back my changes<br class="">2. Implemented all the tests presented in the most recent patch<br class="">3. Implemented ‘operator==‘ to return false -> some ‘==‘-related tests failed<br class="">4. Implemented ‘operator==‘ as negation of ‘operator!=‘ (i.e.: ‘!(*this != Other)') -> tests passed<br class="">5. Moved inverted implementation of ‘operator!=‘ into ‘operator==‘ -> tests passed<br class="">6. Implemented ‘operator!=‘ as negation of ‘operator==‘ (i.e.: ‘!(*this == Other)’) -> tests passed<br class=""><br class="">So I still didn’t get what is wrong with the implementation.<br class=""><br class="">I would appreciate if you can shed more light on the problem.<br class="">--<br class="">AlexDenisov<br class="">Software Engineer, <a href="http://lowlevelbits.org" class="">http://lowlevelbits.org</a><br class=""><br class=""><blockquote type="cite" class="">On 17 Dec 2015, at 03:46, Jordan Rose <<a href="mailto:jordan_rose@apple.com" class="">jordan_rose@apple.com</a>> wrote:<br class=""><br class="">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.<br class=""><br class="">Besides that, though, the tests look good! Thanks for doing this.<br class="">Jordan<br class=""><br class=""><blockquote type="cite" class="">On Dec 16, 2015, at 14:03 , Alexey Denisov <<a href="mailto:1101.debian@gmail.com" class="">1101.debian@gmail.com</a>> wrote:<br class=""><br class="">Hmm, I did add couple of tests, but can’t catch wrong behaviour.<br class="">What am I missing here?..<br class=""><br class=""><basic_collection_iterator_operator_equals.patch><br class=""><br class="">--<br class="">AlexDenisov<br class="">Software Engineer, <a href="http://lowlevelbits.org" class="">http://lowlevelbits.org</a><br class=""><br class=""><blockquote type="cite" class="">On 15 Dec 2015, at 23:59, Jordan Rose <<a href="mailto:jordan_rose@apple.com" class="">jordan_rose@apple.com</a>> wrote:<br class=""><br class="">The logic here is wrong:<br class=""><br class=""><blockquote type="cite" class="">+ bool operator==(const basic_collection_iterator &Other) const {<br class="">+ if (Base == Other.Base)<br class=""> return true;<br class=""></blockquote><br class="">This test is backwards (early exit on !=, not ==), and the null check below is now incorrect.<br class=""><br class="">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.<br class=""><br class="">Jordan<br class=""><br class=""><br class=""><blockquote type="cite" class="">On Dec 15, 2015, at 13:14 , Alexey Denisov <<a href="mailto:1101.debian@gmail.com" class="">1101.debian@gmail.com</a>> wrote:<br class=""><br class="">Patch adds ‘operator==‘ implementation for ‘basic_collection_iterator’<br class=""><br class="">Motivation:<br class=""><br class="">Swift compiler uses workaround since 'operator==‘ is not implemented.<br class=""><br class="">Driver.cpp:220<br class=""><br class="">if (!(seqI != seqE))<br class="">return true;<br class=""><br class=""><br class=""><br class=""><basic_collection_iterator_operator_equals.patch><br class=""><br class="">--<br class="">AlexDenisov<br class="">Software Engineer, <a href="http://lowlevelbits.org" class="">http://lowlevelbits.org</a><br class=""><br class=""></blockquote><br class=""></blockquote><br class=""></blockquote><br class=""></blockquote><br class=""></blockquote><br class=""></blockquote><br class=""></div></div></blockquote></div><br class=""></body></html>