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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
