[Lldb-commits] [PATCH] D94136: [lldb] Make ModuleList iterable (NFC)

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.


================
Comment at: lldb/include/lldb/Core/ModuleList.h:71
+    : public llvm::iterator_facade_base<
+          ModuleIterator, std::bidirectional_iterator_tag, lldb::ModuleSP> {
+public:
----------------
On the fence, but this could be a random access iterator, since it's only based on an integer index - would be fairly easy to implement, but I can appreciate not wanting to add more iterator surface area than you need for range-based-for loops.


================
Comment at: lldb/include/lldb/Core/ModuleList.h:76
+
+  const lldb::ModuleSP operator*() const;
+
----------------
Returning const value types is particularly weird/problematic in some ways - it prevents any moving from the return value. So should probably drop the "const" here.


================
Comment at: lldb/include/lldb/Core/ModuleList.h:93-96
+  friend bool operator!=(const ModuleIterator &lhs, const ModuleIterator &rhs) {
+    return !(lhs == rhs);
+  }
+
----------------
Seems you don't need to implement `!=` - iterator_facade_base will provide that for you based on op==, I think?


================
Comment at: lldb/include/lldb/Core/ModuleList.h:98
+private:
+  const ModuleList &m_module_list;
+  size_t m_index;
----------------
This being a reference (rather than, say, a pointer) would prevent the iterator from being assignable, I think? Which isn't ideal for many iterator algorithms, etc.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94136/new/

https://reviews.llvm.org/D94136