[llvm] r294349 - ADT: Add explicit conversions for reverse ilist iterators
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 7 18:00:00 PST 2017
It's fixing an API regression; previously, ilist::reverse_iterator had an explicit conversion constructor (with these semantics) from ilist::iterator. Since I removed it, there have been a number of complaints that it *should* exist in its old form.
Those upgrading from LLVM 3.9 to LLVM 4.0 will have less code to update if we take this patch, and the instructions for how to update it are simpler as a result of it.
> On 2017-Feb-07, at 15:03, Hans Wennborg <hans at chromium.org> wrote:
>
> Since it's not fixing a regression from 3.9, I'd prefer not to merge
> it at this stage, but let it be part of the next release instead.
>
> On Tue, Feb 7, 2017 at 2:48 PM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
>> Hans, can we take this commit (r294349) and David's fixup in r294357 into the LLVM 4.0 branch?
>>
>> I've also attached a patch for the release notes that attempt to summarize the ilist changes.
>>
>> Thanks!
>> Duncan
>>
>>
>>
>>
>>> On 2017-Feb-07, at 13:03, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>>
>>> Author: dexonsmith
>>> Date: Tue Feb 7 15:03:50 2017
>>> New Revision: 294349
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=294349&view=rev
>>> Log:
>>> ADT: Add explicit conversions for reverse ilist iterators
>>>
>>> Add explicit conversions between forward and reverse ilist iterators.
>>> These follow the conversion conventions of std::reverse_iterator, which
>>> are off-by-one: the newly-constructed "reverse" iterator dereferences to
>>> the previous node of the one sent in. This has the benefit of
>>> converting reverse ranges in place:
>>> - If [I, E) is a valid range,
>>> - then [reverse(E), reverse(I)) gives the same range in reverse order.
>>>
>>> ilist_iterator::getReverse() is unchanged: it returns a reverse iterator
>>> to the *same* node.
>>>
>>> Modified:
>>> llvm/trunk/include/llvm/ADT/ilist_iterator.h
>>> llvm/trunk/include/llvm/CodeGen/MachineInstrBundleIterator.h
>>> llvm/trunk/unittests/ADT/IListIteratorTest.cpp
>>> llvm/trunk/unittests/CodeGen/MachineInstrBundleIteratorTest.cpp
>>>
>>> Modified: llvm/trunk/include/llvm/ADT/ilist_iterator.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/ilist_iterator.h?rev=294349&r1=294348&r2=294349&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/ADT/ilist_iterator.h (original)
>>> +++ llvm/trunk/include/llvm/ADT/ilist_iterator.h Tue Feb 7 15:03:50 2017
>>> @@ -102,10 +102,23 @@ public:
>>> return *this;
>>> }
>>>
>>> - /// Convert from an iterator to its reverse.
>>> + /// Explicit conversion between forward/reverse iterators.
>>> ///
>>> - /// TODO: Roll this into the implicit constructor once we're sure that no one
>>> - /// is relying on the std::reverse_iterator off-by-one semantics.
>>> + /// Translate between forward and reverse iterators without changing range
>>> + /// boundaries. The resulting iterator will dereference (and have a handle)
>>> + /// to the previous node, which is somewhat unexpected; but converting the
>>> + /// two endpoints in a range will give the same range in reverse.
>>> + ///
>>> + /// This matches std::reverse_iterator conversions.
>>> + explicit ilist_iterator(
>>> + const ilist_iterator<OptionsT, !IsReverse, IsConst> &RHS)
>>> + : ilist_iterator(++RHS.getReverse()) {}
>>> +
>>> + /// Get a reverse iterator to the same node.
>>> + ///
>>> + /// Gives a reverse iterator that will dereference (and have a handle) to the
>>> + /// same node. Converting the endpoint iterators in a range will give a
>>> + /// different range; for range operations, use the explicit conversions.
>>> ilist_iterator<OptionsT, !IsReverse, IsConst> getReverse() const {
>>> if (NodePtr)
>>> return ilist_iterator<OptionsT, !IsReverse, IsConst>(*NodePtr);
>>>
>>> Modified: llvm/trunk/include/llvm/CodeGen/MachineInstrBundleIterator.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/MachineInstrBundleIterator.h?rev=294349&r1=294348&r2=294349&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/CodeGen/MachineInstrBundleIterator.h (original)
>>> +++ llvm/trunk/include/llvm/CodeGen/MachineInstrBundleIterator.h Tue Feb 7 15:03:50 2017
>>> @@ -153,6 +153,18 @@ public:
>>> : MII(I.getInstrIterator()) {}
>>> MachineInstrBundleIterator() : MII(nullptr) {}
>>>
>>> + /// Explicit conversion between forward/reverse iterators.
>>> + ///
>>> + /// Translate between forward and reverse iterators without changing range
>>> + /// boundaries. The resulting iterator will dereference (and have a handle)
>>> + /// to the previous node, which is somewhat unexpected; but converting the
>>> + /// two endpoints in a range will give the same range in reverse.
>>> + ///
>>> + /// This matches std::reverse_iterator conversions.
>>> + explicit MachineInstrBundleIterator(
>>> + const MachineInstrBundleIterator<Ty, !IsReverse> &I)
>>> + : MachineInstrBundleIterator(++I.getReverse()) {}
>>> +
>>> /// Get the bundle iterator for the given instruction's bundle.
>>> static MachineInstrBundleIterator getAtBundleBegin(instr_iterator MI) {
>>> return MachineInstrBundleIteratorHelper<IsReverse>::getBundleBegin(MI);
>>> @@ -258,6 +270,11 @@ public:
>>>
>>> nonconst_iterator getNonConstIterator() const { return MII.getNonConst(); }
>>>
>>> + /// Get a reverse iterator to the same node.
>>> + ///
>>> + /// Gives a reverse iterator that will dereference (and have a handle) to the
>>> + /// same node. Converting the endpoint iterators in a range will give a
>>> + /// different range; for range operations, use the explicit conversions.
>>> reverse_iterator getReverse() const { return MII.getReverse(); }
>>> };
>>>
>>>
>>> Modified: llvm/trunk/unittests/ADT/IListIteratorTest.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/IListIteratorTest.cpp?rev=294349&r1=294348&r2=294349&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/unittests/ADT/IListIteratorTest.cpp (original)
>>> +++ llvm/trunk/unittests/ADT/IListIteratorTest.cpp Tue Feb 7 15:03:50 2017
>>> @@ -131,4 +131,44 @@ TEST(IListIteratorTest, CheckEraseRevers
>>> EXPECT_EQ(L.rend(), RI);
>>> }
>>>
>>> +TEST(IListIteratorTest, ReverseConstructor) {
>>> + simple_ilist<Node> L;
>>> + const simple_ilist<Node> &CL = L;
>>> + Node A, B;
>>> + L.insert(L.end(), A);
>>> + L.insert(L.end(), B);
>>> +
>>> + // Save typing.
>>> + typedef simple_ilist<Node>::iterator iterator;
>>> + typedef simple_ilist<Node>::reverse_iterator reverse_iterator;
>>> + typedef simple_ilist<Node>::const_iterator const_iterator;
>>> + typedef simple_ilist<Node>::const_reverse_iterator const_reverse_iterator;
>>> +
>>> + // Check conversion values.
>>> + EXPECT_EQ(L.begin(), iterator(L.rend()));
>>> + EXPECT_EQ(++L.begin(), iterator(++L.rbegin()));
>>> + EXPECT_EQ(L.end(), iterator(L.rbegin()));
>>> + EXPECT_EQ(L.rbegin(), reverse_iterator(L.end()));
>>> + EXPECT_EQ(++L.rbegin(), reverse_iterator(++L.begin()));
>>> + EXPECT_EQ(L.rend(), reverse_iterator(L.begin()));
>>> +
>>> + // Check const iterator constructors.
>>> + EXPECT_EQ(CL.begin(), const_iterator(L.rend()));
>>> + EXPECT_EQ(CL.begin(), const_iterator(CL.rend()));
>>> + EXPECT_EQ(CL.rbegin(), const_reverse_iterator(L.end()));
>>> + EXPECT_EQ(CL.rbegin(), const_reverse_iterator(CL.end()));
>>> +
>>> + // Confirm lack of implicit conversions.
>>> + static_assert(std::is_convertible<iterator, reverse_iterator>::value,
>>> + "unexpected implicit conversion");
>>> + static_assert(std::is_convertible<reverse_iterator, iterator>::value,
>>> + "unexpected implicit conversion");
>>> + static_assert(
>>> + std::is_convertible<const_iterator, const_reverse_iterator>::value,
>>> + "unexpected implicit conversion");
>>> + static_assert(
>>> + std::is_convertible<const_reverse_iterator, const_iterator>::value,
>>> + "unexpected implicit conversion");
>>> +}
>>> +
>>> } // end namespace
>>>
>>> Modified: llvm/trunk/unittests/CodeGen/MachineInstrBundleIteratorTest.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/CodeGen/MachineInstrBundleIteratorTest.cpp?rev=294349&r1=294348&r2=294349&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/unittests/CodeGen/MachineInstrBundleIteratorTest.cpp (original)
>>> +++ llvm/trunk/unittests/CodeGen/MachineInstrBundleIteratorTest.cpp Tue Feb 7 15:03:50 2017
>>> @@ -130,4 +130,68 @@ TEST(MachineInstrBundleIteratorTest, Com
>>> ASSERT_TRUE(CI != CMBI.getIterator());
>>> }
>>>
>>> +struct MyUnbundledInstr
>>> + : ilist_node<MyUnbundledInstr, ilist_sentinel_tracking<true>> {
>>> + bool isBundledWithPred() const { return false; }
>>> + bool isBundledWithSucc() const { return false; }
>>> +};
>>> +typedef MachineInstrBundleIterator<MyUnbundledInstr> unbundled_iterator;
>>> +typedef MachineInstrBundleIterator<const MyUnbundledInstr>
>>> + const_unbundled_iterator;
>>> +typedef MachineInstrBundleIterator<MyUnbundledInstr, true>
>>> + reverse_unbundled_iterator;
>>> +typedef MachineInstrBundleIterator<const MyUnbundledInstr, true>
>>> + const_reverse_unbundled_iterator;
>>> +
>>> +TEST(MachineInstrBundleIteratorTest, ReverseConstructor) {
>>> + simple_ilist<MyUnbundledInstr, ilist_sentinel_tracking<true>> L;
>>> + const auto &CL = L;
>>> + MyUnbundledInstr A, B;
>>> + L.insert(L.end(), A);
>>> + L.insert(L.end(), B);
>>> +
>>> + // Save typing.
>>> + typedef MachineInstrBundleIterator<MyUnbundledInstr> iterator;
>>> + typedef MachineInstrBundleIterator<MyUnbundledInstr, true> reverse_iterator;
>>> + typedef MachineInstrBundleIterator<const MyUnbundledInstr> const_iterator;
>>> + typedef MachineInstrBundleIterator<const MyUnbundledInstr, true>
>>> + const_reverse_iterator;
>>> +
>>> + // Convert to bundle iterators.
>>> + auto begin = [&]() -> iterator { return L.begin(); };
>>> + auto end = [&]() -> iterator { return L.end(); };
>>> + auto rbegin = [&]() -> reverse_iterator { return L.rbegin(); };
>>> + auto rend = [&]() -> reverse_iterator { return L.rend(); };
>>> + auto cbegin = [&]() -> const_iterator { return CL.begin(); };
>>> + auto cend = [&]() -> const_iterator { return CL.end(); };
>>> + auto crbegin = [&]() -> const_reverse_iterator { return CL.rbegin(); };
>>> + auto crend = [&]() -> const_reverse_iterator { return CL.rend(); };
>>> +
>>> + // Check conversion values.
>>> + EXPECT_EQ(begin(), iterator(rend()));
>>> + EXPECT_EQ(++begin(), iterator(++rbegin()));
>>> + EXPECT_EQ(end(), iterator(rbegin()));
>>> + EXPECT_EQ(rbegin(), reverse_iterator(end()));
>>> + EXPECT_EQ(++rbegin(), reverse_iterator(++begin()));
>>> + EXPECT_EQ(rend(), reverse_iterator(begin()));
>>> +
>>> + // Check const iterator constructors.
>>> + EXPECT_EQ(cbegin(), const_iterator(rend()));
>>> + EXPECT_EQ(cbegin(), const_iterator(crend()));
>>> + EXPECT_EQ(crbegin(), const_reverse_iterator(end()));
>>> + EXPECT_EQ(crbegin(), const_reverse_iterator(cend()));
>>> +
>>> + // Confirm lack of implicit conversions.
>>> + static_assert(!std::is_convertible<iterator, reverse_iterator>::value,
>>> + "unexpected implicit conversion");
>>> + static_assert(!std::is_convertible<reverse_iterator, iterator>::value,
>>> + "unexpected implicit conversion");
>>> + static_assert(
>>> + !std::is_convertible<const_iterator, const_reverse_iterator>::value,
>>> + "unexpected implicit conversion");
>>> + static_assert(
>>> + !std::is_convertible<const_reverse_iterator, const_iterator>::value,
>>> + "unexpected implicit conversion");
>>> +}
>>> +
>>> } // end namespace
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
More information about the llvm-commits
mailing list