[PATCH] D49679: [DebugInfo] Have custom DWARFDie:: reverse_iterator

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

I like this, but that's not surprising, as it was my idea in the first place. :)

I think David should have a pass at this too.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h:373
+namespace std {
+using namespace llvm;
+template <>
----------------
This will inject all llvm decls into the `std` namespace. bad idea :)


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h:389-392
+  typedef typename iterator_traits<DWARFDie::iterator>::difference_type
+      difference_type;
+  typedef typename iterator_traits<DWARFDie::iterator>::reference reference;
+  typedef typename iterator_traits<DWARFDie::iterator>::pointer pointer;
----------------
These should be already inherited via std::iterator, shouldn't they?


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h:410-418
+  reverse_iterator<DWARFDie::iterator> &operator--() {
+    if (AtEnd) {
+      AtEnd = false;
+      return *this;
+    }
+    Die = Die.getSibling();
+    assert(!Die.isNULL() && "Decrementing rbegin");
----------------
We should also add a test which does a manual reverse iteration (via this function) on the reverse_iterator :), to check that this works too.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h:420
+
+  explicit operator bool() const { return Die.isValid(); }
+  const DWARFDie &operator*() const { return Die; }
----------------
This should also take the value of `AtEnd` into account. Probably something like `Die.isValid() && !AtEnd`. Maybe you could even assert `AtEnd == false` when dereferencing.


https://reviews.llvm.org/D49679