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

David Blaikie via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Tue Jan 5 17:08:49 PST 2021

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> {
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
+  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.



More information about the lldb-commits mailing list