[Lldb-commits] [PATCH] D64013: Correctly use GetLoadedModuleList to take advantage of libraries-svr4

[Pavel Labath via Phabricator via lldb-commits]

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