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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
