[PATCH] D49679: [DebugInfo] Have custom std::reverse_iterator<DWARFDie>

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

(probably a separate patch) Wonder if we could have a specialization of llvm::reverse that detects whether the underlying range has 'rbegin/rend' functions and use those, rather than making reverse iterators out of end/begin. That way reverse(die) would work correctly.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h:393-394
+
+bool operator==(const reverse_iterator<llvm::DWARFDie::iterator> &LHS,
+                const reverse_iterator<llvm::DWARFDie::iterator> &RHS);
+template <>
----------------
/looks/ to me like this declaration isn't needed? I don't see any use of it in the specialization/before the op== definition later.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h:397-402
+    : public iterator<
+          typename iterator_traits<llvm::DWARFDie::iterator>::iterator_category,
+          typename iterator_traits<llvm::DWARFDie::iterator>::value_type,
+          typename iterator_traits<llvm::DWARFDie::iterator>::difference_type,
+          typename iterator_traits<llvm::DWARFDie::iterator>::pointer,
+          typename iterator_traits<llvm::DWARFDie::iterator>::reference> {
----------------
Any chance of using llvm's iterator_facade here, might simplify things a bit?


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h:447-456
+inline bool operator==(const reverse_iterator<llvm::DWARFDie::iterator> &LHS,
+                       const reverse_iterator<llvm::DWARFDie::iterator> &RHS) {
+  return LHS.equals(RHS);
+}
+
+inline bool operator!=(const reverse_iterator<llvm::DWARFDie::iterator> &LHS,
+                       const reverse_iterator<llvm::DWARFDie::iterator> &RHS) {
----------------
I wonder if these should go in the llvm namespace (they'd still be found by ADL, I think) rather than here in the std namespace?


https://reviews.llvm.org/D49679