[llvm] r294349 - ADT: Add explicit conversions for reverse ilist iterators

Hans Wennborg via llvm-commits llvm-commits at lists.llvm.org
Tue Feb 7 15:03:13 PST 2017


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