owenpshaw added a comment. Thanks! Overall it's feeling like we're getting down to mainly differences in personal preferences and judgements. Not sure if you're looking for a discussion, which I'm happy to have, if you're just looking for changes to definitely be made. If it's the latter, I think it'd be more efficient to just hand this off so the changes can be made without a lot of back and forth.
================ Comment at: include/lldb/Target/Process.h:1958 + virtual bool WriteObjectFile(llvm::ArrayRef<WriteEntry> entries, + Status &error); ---------------- labath wrote: > 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`). Open to whatever. I preferred how it made calling code a little simpler. ``` if (!WriteObjectFile(entries, error)) return error; ``` rather than ``` error = WriteObjectFile(entries); if (!error.Sucess()) return 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), ---------------- labath wrote: > 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)`). I didn't love the copy either, but figured 1) it was pretty cheap given the expected values 2) it eliminates an unexpected modification of the passed array. I prefer having the sort in the process file, since it's really the only scope that knows about the sorting requirement. ================ 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 ---------------- labath wrote: > 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). Guess we have different definitions of messy :) Wouldn't any other approach pretty much require duplicating WriteMemory, WriteMemoryPrivate, and DoWriteMemory? - It seemed like the breakpoint logic in WriteMemory could be important when writing an object to ram, so I wanted to preserve that - The loop in WriteMemoryPrivate is fairly trivial, but why add a second one if not needed? - DoWriteMemory is mostly code that is common to both ram and flash writes, especially if a ram-only version would still need to check if address is flash so it could report an error. https://reviews.llvm.org/D42145 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
