labath added a comment. Looks mostly good. Just a couple of style comments.
================ Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:185-207 + if ( + // When the previous and current states are consistent this is the first + // time we have been asked to update. Just take a snapshot of the + // currently loaded modules. + (m_previous.state == eConsistent && m_current.state == eConsistent) || + // If we are about to add or remove a shared object clear out the current + // state and take a snapshot of the currently loaded images. ---------------- This logic is pretty hard to follow. I'd suggest trying to structure as some switch statements. Something like: ``` switch(current_state) { case Consistent: switch (previous_state) { case Consistent: return TakeSnapshot; case Add: return Add; case Remove: return Remove; } case Add: case Remove: @#%@#%!@% } ``` ================ Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:240 +bool DYLDRendezvous::UpdateSOEntries() { + auto action = GetAction(); ---------------- Use a switch here. ================ Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h:265 + /// state + enum RendezvousAction GetAction() const; }; ---------------- We don't usually use the enum tag here. ================ Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2731 if (addr == LLDB_INVALID_ADDRESS) { - LoadedModuleInfoList list; - if (GetLoadedModuleList(list).Success()) - addr = list.m_link_map; + llvm::Expected<LoadedModuleInfoList> list = GetLoadedModuleList(); + if (list) ---------------- You'll need to somehow handle the case when GetLoadedModuleList fails (maybe by logging the error). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64013/new/ https://reviews.llvm.org/D64013 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits