Page MenuHomePhabricator

[lldb] Make ModuleList iterable (NFC)

Authored by JDevlieghere on Tue, Jan 5, 4:03 PM.


Group Reviewers
Restricted Project

Add begin and end iterators to the ModuleList so we can use range-based for loops without locking from beginning to end. This is what makes it different from the Modules() method which returns a LockingAdaptedIterable but also means the burden of correctness is on the caller.

I'm not sure if this distinction matters, but it preserves the semantics of the code that is currently using GetModuleAtIndex directly from inside a classically indexed for loop.

Diff Detail

Event Timeline

JDevlieghere created this revision.Tue, Jan 5, 4:03 PM
JDevlieghere requested review of this revision.Tue, Jan 5, 4:03 PM
JDevlieghere edited the summary of this revision. (Show Details)Tue, Jan 5, 4:22 PM
dblaikie added inline comments.

FWIW, std::iterator is deprecated since C++17 - probably best not to add new uses of it. (I think the idea is that the typedefs that std::iterator provides are no longer needed because of auto/decltype/etc, but I might be wrong there (would have to check the iterator concepts specifications))

If you need some utility help with implementing an iterator, llvm's iterator_facade_base might help by allowing a fairly minimal implementation to autogenerate various symmetric members, etc.

JDevlieghere marked an inline comment as done.

Use iterator_facade_base as suggested by @dblaikie.

dblaikie added inline comments.Tue, Jan 5, 5:08 PM

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.


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.


Seems you don't need to implement != - iterator_facade_base will provide that for you based on op==, I think?


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.

JDevlieghere marked 4 inline comments as done.

Address Dave's feedback.

dblaikie accepted this revision.Tue, Jan 5, 6:51 PM

Looks good to me with a few minor cleanups.


Interesting choice to make the single-arg ctor implicit but the multi-arg explicit (usually people end up the other way around - because until C++ 17 or something, explicit was only relevant for single-arg constructors). In any case the ctors are only used once each & probably might as well have them both explicit.

Or maybe make only one ctor that takes the index explicitly and use it from begin/end - not sure there's much value added by having two ctors here, and honestly I'd have expected the no-arg version to start at zero, and the arg-taking version to start wherever you want, and I see they're the opposite, so that'd probably reinforce the idea that just having the one that takes an explicit index might be less confusing.


I think you can drop these two now - they'll be provided by the iterator facade.

This revision is now accepted and ready to land.Tue, Jan 5, 6:51 PM
JDevlieghere added inline comments.Tue, Jan 5, 7:40 PM

I copied all of this from somewhere else in LLDB without giving it much thought because honestly I'm not sure this iterator is that much better than then existing LockingAdaptedIterable. Sorry for making you review this half-baked code...

JDevlieghere abandoned this revision.Thu, Jan 7, 3:57 PM

Abandoning this in favor of using the ModuleIterable everywhere: