hanchenye-llvm-project/llvm/unittests/ADT/IListIteratorTest.cpp

175 lines
4.9 KiB
C++
Raw Normal View History

ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
//===- unittests/ADT/IListIteratorTest.cpp - ilist_iterator unit tests ----===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
#include "llvm/ADT/simple_ilist.h"
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
#include "gtest/gtest.h"
using namespace llvm;
namespace {
struct Node : ilist_node<Node> {};
TEST(IListIteratorTest, DefaultConstructor) {
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
simple_ilist<Node>::iterator I;
simple_ilist<Node>::reverse_iterator RI;
simple_ilist<Node>::const_iterator CI;
simple_ilist<Node>::const_reverse_iterator CRI;
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
EXPECT_EQ(nullptr, I.getNodePtr());
EXPECT_EQ(nullptr, CI.getNodePtr());
EXPECT_EQ(nullptr, RI.getNodePtr());
EXPECT_EQ(nullptr, CRI.getNodePtr());
EXPECT_EQ(I, I);
EXPECT_EQ(I, CI);
EXPECT_EQ(CI, I);
EXPECT_EQ(CI, CI);
EXPECT_EQ(RI, RI);
EXPECT_EQ(RI, CRI);
EXPECT_EQ(CRI, RI);
EXPECT_EQ(CRI, CRI);
EXPECT_EQ(I, RI.getReverse());
EXPECT_EQ(RI, I.getReverse());
}
TEST(IListIteratorTest, Empty) {
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
simple_ilist<Node> L;
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
// Check iterators of L.
EXPECT_EQ(L.begin(), L.end());
EXPECT_EQ(L.rbegin(), L.rend());
// Reverse of end should be rend (since the sentinel sits on both sides).
EXPECT_EQ(L.end(), L.rend().getReverse());
EXPECT_EQ(L.rend(), L.end().getReverse());
// Iterators shouldn't match default constructors.
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
simple_ilist<Node>::iterator I;
simple_ilist<Node>::reverse_iterator RI;
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
EXPECT_NE(I, L.begin());
EXPECT_NE(I, L.end());
EXPECT_NE(RI, L.rbegin());
EXPECT_NE(RI, L.rend());
}
TEST(IListIteratorTest, OneNodeList) {
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
simple_ilist<Node> L;
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
Node A;
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
L.insert(L.end(), A);
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
// Check address of reference.
EXPECT_EQ(&A, &*L.begin());
EXPECT_EQ(&A, &*L.rbegin());
// Check that the handle matches.
EXPECT_EQ(L.rbegin().getNodePtr(), L.begin().getNodePtr());
// Check iteration.
EXPECT_EQ(L.end(), ++L.begin());
EXPECT_EQ(L.begin(), --L.end());
EXPECT_EQ(L.rend(), ++L.rbegin());
EXPECT_EQ(L.rbegin(), --L.rend());
// Check conversions.
EXPECT_EQ(L.rbegin(), L.begin().getReverse());
EXPECT_EQ(L.begin(), L.rbegin().getReverse());
}
TEST(IListIteratorTest, TwoNodeList) {
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
simple_ilist<Node> L;
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
Node A, B;
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
L.insert(L.end(), A);
L.insert(L.end(), B);
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
// Check order.
EXPECT_EQ(&A, &*L.begin());
EXPECT_EQ(&B, &*++L.begin());
EXPECT_EQ(L.end(), ++++L.begin());
EXPECT_EQ(&B, &*L.rbegin());
EXPECT_EQ(&A, &*++L.rbegin());
EXPECT_EQ(L.rend(), ++++L.rbegin());
// Check conversions.
EXPECT_EQ(++L.rbegin(), L.begin().getReverse());
EXPECT_EQ(L.rbegin(), (++L.begin()).getReverse());
EXPECT_EQ(++L.begin(), L.rbegin().getReverse());
EXPECT_EQ(L.begin(), (++L.rbegin()).getReverse());
}
TEST(IListIteratorTest, CheckEraseForward) {
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
simple_ilist<Node> L;
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
Node A, B;
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
L.insert(L.end(), A);
L.insert(L.end(), B);
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
// Erase nodes.
auto I = L.begin();
EXPECT_EQ(&A, &*I);
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
L.remove(*I++);
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
EXPECT_EQ(&B, &*I);
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
L.remove(*I++);
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
EXPECT_EQ(L.end(), I);
}
TEST(IListIteratorTest, CheckEraseReverse) {
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
simple_ilist<Node> L;
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
Node A, B;
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
L.insert(L.end(), A);
L.insert(L.end(), B);
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
// Erase nodes.
auto RI = L.rbegin();
EXPECT_EQ(&B, &*RI);
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
L.remove(*RI++);
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
EXPECT_EQ(&A, &*RI);
ADT: Split out simple_ilist, a simple intrusive list Split out a new, low-level intrusive list type with clear semantics. Unlike iplist (and ilist), all operations on simple_ilist are intrusive, and simple_ilist never takes ownership of its nodes. This enables an intuitive API that has the right defaults for intrusive lists. - insert() takes references (not pointers!) to nodes (in iplist/ilist, passing a reference will cause the node to be copied). - erase() takes only iterators (like std::list), and does not destroy the nodes. - remove() takes only references and has the same behaviour as erase(). - clear() does not destroy the nodes. - The destructor does not destroy the nodes. - New API {erase,remove,clear}AndDispose() take an extra Disposer functor for callsites that want to call some disposal routine (e.g., std::default_delete). This list is not currently configurable, and has no callbacks. The initial motivation was to fix iplist<>::sort to work correctly (even with callbacks in ilist_traits<>). iplist<> uses simple_ilist<>::sort directly. The new test in unittests/IR/ModuleTest.cpp crashes without this commit. Fixing sort() via a low-level layer provided a good opportunity to: - Unit test the low-level functionality thoroughly. - Modernize the API, largely inspired by other intrusive list implementations. Here's a sketch of a longer-term plan: - Create BumpPtrList<>, a non-intrusive list implemented using simple_ilist<>, and use it for the Token list in lib/Support/YAMLParser.cpp. This will factor out the only real use of createNode(). - Evolve the iplist<> and ilist<> APIs in the direction of simple_ilist<>, making allocation/deallocation explicit at call sites (similar to simple_ilist<>::eraseAndDispose()). - Factor out remaining calls to createNode() and deleteNode() and remove the customization from ilist_traits<>. - Transition uses of iplist<>/ilist<> that don't need callbacks over to simple_ilist<>. llvm-svn: 280107
2016-08-31 00:23:55 +08:00
L.remove(*RI++);
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
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");
}
ADT: Give ilist<T>::reverse_iterator a handle to the current node Reverse iterators to doubly-linked lists can be simpler (and cheaper) than std::reverse_iterator. Make it so. In particular, change ilist<T>::reverse_iterator so that it is *never* invalidated unless the node it references is deleted. This matches the guarantees of ilist<T>::iterator. (Note: MachineBasicBlock::iterator is *not* an ilist iterator, but a MachineInstrBundleIterator<MachineInstr>. This commit does not change MachineBasicBlock::reverse_iterator, but it does update MachineBasicBlock::reverse_instr_iterator. See note at end of commit message for details on bundle iterators.) Given the list (with the Sentinel showing twice for simplicity): [Sentinel] <-> A <-> B <-> [Sentinel] the following is now true: 1. begin() represents A. 2. begin() holds the pointer for A. 3. end() represents [Sentinel]. 4. end() holds the poitner for [Sentinel]. 5. rbegin() represents B. 6. rbegin() holds the pointer for B. 7. rend() represents [Sentinel]. 8. rend() holds the pointer for [Sentinel]. The changes are #6 and #8. Here are some properties from the old scheme (which used std::reverse_iterator): - rbegin() held the pointer for [Sentinel] and rend() held the pointer for A; - operator*() cost two dereferences instead of one; - converting from a valid iterator to its valid reverse_iterator involved a confusing increment; and - "RI++->erase()" left RI invalid. The unintuitive replacement was "RI->erase(), RE = end()". With vector-like data structures these properties are hard to avoid (since past-the-beginning is not a valid pointer), and don't impose a real cost (since there's still only one dereference, and all iterators are invalidated on erase). But with lists, this was a poor design. Specifically, the following code (which obviously works with normal iterators) now works with ilist::reverse_iterator as well: for (auto RI = L.rbegin(), RE = L.rend(); RI != RE;) fooThatMightRemoveArgFromList(*RI++); Converting between iterator and reverse_iterator for the same node uses the getReverse() function. reverse_iterator iterator::getReverse(); iterator reverse_iterator::getReverse(); Why doesn't iterator <=> reverse_iterator conversion use constructors? In order to catch and update old code, reverse_iterator does not even have an explicit conversion from iterator. It wouldn't be safe because there would be no reasonable way to catch all the bugs from the changed semantic (see the changes at call sites that are part of this patch). Old code used this API: std::reverse_iterator::reverse_iterator(iterator); iterator std::reverse_iterator::base(); Here's how to update from old code to new (that incorporates the semantic change), assuming I is an ilist<>::iterator and RI is an ilist<>::reverse_iterator: [Old] ==> [New] reverse_iterator(I) (--I).getReverse() reverse_iterator(I) ++I.getReverse() --reverse_iterator(I) I.getReverse() reverse_iterator(++I) I.getReverse() RI.base() (--RI).getReverse() RI.base() ++RI.getReverse() --RI.base() RI.getReverse() (++RI).base() RI.getReverse() delete &*RI, RE = end() delete &*RI++ RI->erase(), RE = end() RI++->erase() ======================================= Note: bundle iterators are out of scope ======================================= MachineBasicBlock::iterator, also known as MachineInstrBundleIterator<MachineInstr>, is a wrapper to represent MachineInstr bundles. The idea is that each operator++ takes you to the beginning of the next bundle. Implementing a sane reverse iterator for this is harder than ilist. Here are the options: - Use std::reverse_iterator<MBB::i>. Store a handle to the beginning of the next bundle. A call to operator*() runs a loop (usually operator--() will be called 1 time, for unbundled instructions). Increment/decrement just works. This is the status quo. - Store a handle to the final node in the bundle. A call to operator*() still runs a loop, but it iterates one time fewer (usually operator--() will be called 0 times, for unbundled instructions). Increment/decrement just works. - Make the ilist_sentinel<MachineInstr> *always* store that it's the sentinel (instead of just in asserts mode). Then the bundle iterator can sniff the sentinel bit in operator++(). I initially tried implementing the end() option as part of this commit, but updating iterator/reverse_iterator conversion call sites was error-prone. I have a WIP series of patches that implements the final option. llvm-svn: 280032
2016-08-30 08:13:12 +08:00
} // end namespace