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

[António Afonso via Phabricator via lldb-commits]

aadsm marked 2 inline comments as done.
aadsm added inline comments.


================
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"); }
 
----------------
labath wrote:
> Since you're already changing the signature, let's make this return an llvm::Error.
Ah yeah ok. It still weirds me out that an Error object might not be an error. In this regard I actually much prefer the Status object as it much better conveys the meaning of the return object. Is the plan to move from Status to Error in lldb?


================
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
----------------
labath wrote:
> 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...
Sounds good. I didn't want to use the word consistent, I wanted to match rest of the language used in the code. Originally I had ShouldX (e.g.: ShouldTakeSnapshot) but I didn't think it was clear when to call it (or that it was derived from the state). I like the GetAction idea, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64013