labath added a comment.

Yes, it's mostly stylistic differences, but there is one more important thing 
that we need to figure out, and that's making sure that the test only runs if 
we actually have XML support compiled-in. If there's isn't anything in the SB 
API that would tell us that (I cursory look doesn't reveal anything), then we 
may need to get some help from the build system for that. OTOH, maybe an new SB 
API for determining the lldb feature set would actually be useful...

Comment at: include/lldb/Target/Process.h:1958
+  virtual bool WriteObjectFile(llvm::ArrayRef<WriteEntry> entries,
+                               Status &error);
owenpshaw wrote:
> 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
> ```
That extra line is a bit of a nuissance. We could fix that, but as the long 
term solution is to use llvm::Error everywhere, i'm not motivated to do that. 
So if the extra line bothers you, just use llvm::Error. Then the call-size 
if (llvm::Error E = WriteObjectFile(...))
  return Status(std::move(E));
The constant conversion between Status and Error is a bit annoying, but as more 
places start using this, it should get better.

(My main issue with your syntax is that it's not DRY)

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),
owenpshaw wrote:
> 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. 
I agree it likely to be cheap, but the thing you fear (2) can be easily avoided.
Note that I deliberately did not add any reference qualifications to the type 
in the comment above. If you take the argument as a value type, then you leave 
it up to the caller to decide whether a copy takes place.
If he calls it as `WriteMemoryObject(my_vector)`, then it's copied (and the 
original is left unmodified).
If he calls it as `WriteMemoryObject(std::move(my_vector))`, then it's moved 
(but that's very obvious from looking at the call-site).

So in the worst case, you get one copy, just like you would here, and in the 
best case, you get no copies.
In your case, the caller doesn't need the vector, so we save a copy. To me, 
that makes this option a clear win.
Isn't C++11 great? :D

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
owenpshaw wrote:
> 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.
Yeah, I think I'll have to agree with you here, although this makes the whole 
separate-function approach look far less attractive to me.
If we are going to reuse all of that logic that then we might make it obvious 
that we are doing that by having a bool flag on those functions like Greg 
suggested a couple of comments back.

lldb-commits mailing list

Reply via email to