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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I don't have time to do a full review of this today, but this seems pretty good at a first glance...



================
Comment at: lldb/include/lldb/Target/Process.h:684
+  ///    A status object indicating if the operation was sucessful or not.
+  virtual Status LoadModules() { return Status("Not implemented"); }
 
----------------
Since you're already changing the signature, let's make this return an llvm::Error.


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:184-206
+bool DYLDRendezvous::StateIsTakeSnapshot() {
+  return (
+      // 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
----------------
I like that you've factored out the decoding of the states, but I think it would be better to make this a separate function returning an enum (`GetAction` ?). This will make it more obvious which combinations of flags are covered. Also, `StateIsTakeSnapshot` reads weirdly...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64013