labath added a comment.
I think we're slowly getting there, but we could cleanup the implementation a
bit.
I am also not sure that the `WriteObjectFile` name really captures what this
function does, but I don't have a suggestion for a better name either....
================
Comment at: include/lldb/Target/Process.h:559
+ struct WriteEntry{
+ lldb::addr_t Dest;
----------------
Please run clang format over your diff.
================
Comment at: include/lldb/Target/Process.h:1958
+ virtual bool WriteObjectFile(llvm::ArrayRef<WriteEntry> entries,
+ Status &error);
----------------
This (return bool + by-ref Status) is a bit weird of an api. Could you just
return Status here (but I would not be opposed to going llvm all the way and
returning `llvm::Error`).
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2807
+ // memory, must happen in order of increasing address.
+ std::vector<WriteEntry> sortedEntries(entries);
+ std::stable_sort(std::begin(sortedEntries), std::end(sortedEntries),
----------------
Let's avoid copying the entries here. I can see two options:
- Require that the entries are already sorted on input
- pass them to this function as `std::vector<WriteEntry>` (and then have the
caller call with `std::move(entries)`).
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2812-2821
+ m_allow_flash_writes = true;
+ if (Process::WriteObjectFile(sortedEntries, error))
+ error = FlashDone();
+ else
+ // Even though some of the writing failed, try to send a flash done if
+ // some of the writing succeeded so the flash state is reset to normal,
+ // but don't stomp on the error status that was set in the write failure
----------------
This makes the control flow quite messy. The base function is not so complex
that you have to reuse it at all costs. I think we should just implement the
loop ourselves (and call some write function while passing the
"allow_flash_writes" as an argument).
https://reviews.llvm.org/D42145
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
