[Lldb-commits] [PATCH] D94271: [lldb] Replace GetModuleAtIndexUnlocked and GetModulePointerAtIndexUnlocked with iterators (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib accepted this revision.
mib added a comment.
This revision is now accepted and ready to land.

LGTM apart from few nits.



================
Comment at: lldb/include/lldb/Core/ModuleList.h:485-486
   typedef AdaptedIterable<collection, lldb::ModuleSP, vector_adapter>
       ModuleIterableNoLocking;
-  ModuleIterableNoLocking ModulesNoLocking() {
+  ModuleIterableNoLocking ModulesNoLocking() const {
     return ModuleIterableNoLocking(m_modules);
----------------
nit: Can we rename that to ModuleIterableNo**n**Locking and ModulesNo**n**Locking ?

Sounds more correct to me ^^


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:1404
   std::lock_guard<std::recursive_mutex> guard(module_list.GetMutex());
   const size_t num_modules = module_list.GetSize();
   if (num_modules > 0) {
----------------
Same here, we could remove the check and just use `module_list.GetSize()` in the printf.


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:2279
       std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
       const size_t num_modules = target_modules.GetSize();
       if (num_modules > 0) {
----------------
Same.


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:3911-3914
       } else {
         result.AppendError("the target has no associated executable images");
         result.SetStatus(eReturnStatusFailed);
         return false;
----------------
Early return instead to get rid of `num_modules` ?


================
Comment at: lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp:406
+  Target &target = m_process->GetTarget();
+  const size_t num_modules = target.GetImages().GetSize();
 
----------------
This doesn't seem to be used anymore.


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

https://reviews.llvm.org/D94271