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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

In https://reviews.llvm.org/D49679#1172404, @JDevlieghere wrote:

> In https://reviews.llvm.org/D49679#1172143, @labath wrote:
>
> > What's the reason for not specializing std::reverse_iterator? AFAIK, that is officially permitted by the standard.
>
>
> What exactly would you do difference? I can see a solution where we make the current `__tmp` a class member of the iterator adapter, but maybe you have something more straightforward in mind?
>
> > This way somebody can still construct a `std::reverse_iterator<DWARFDie::iterator>` (via `make_reverse_iterator` or whatever) and get UB.
>
> That's not entirely true because it's no longer a bidirectional iterator. Regardless of the specialization, both `*++iterator` and `*--iterator` will remain UB.


Sorry I wrote the reply without looking at the patch in detail. I did not notice you demoted the iterator back to "forward". In case this is not a bidirectional operator, but a collection of two forward iterators that happen to iterate in the reverse order, the specializing std::reverse_iterator is not the right approach.

I have to admit I haven't thought about the specifics of how the reverse iterator would work until now. However, now that I think about it, I think it could be implemented as a pair of DWARFDie + bool flag. The trick would be that we store the element that we really dereference to, instead of its successor. The bool flag would be used denote the before_begin state, as we cannot represent rend() this way. The implementation would do something like:

  operator++() {
    assert(!AtEnd && "Incrementing rend");
    DWARFDie D = Die.getPreviousSibling();
    if (D)
      Die = D;
    else
      AtEnd = true;
  }
  
  operator--() {
    if (AtEnd) {
      AtEnd = false;
      return;
    }
    Die = Die.getNextSibling();
    assert(!Die.isNULL() && "Decrementing rbegin");
  }

However, if you don't need that extra complexity now, then I suppose this solution is fine too.



================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1081-1116
+  // Make sure iterators work as expected.
+  size_t Idx = 0;
+  for (auto Child : A) {
+    switch (Idx) {
+    case 0:
+      EXPECT_EQ(Child, B);
+      break;
----------------
This will succeed even if the iterator range returns an empty sequence.

Might I suggest a shorter alternative with a more helpful error message?
```
  EXPECT_THAT(std::vector<DWARFDie>(A.begin(), A.end()),
              testing::ElementsAre(B, C, D));
```


https://reviews.llvm.org/D49679