JDevlieghere wrote:
I left a few small comments but overall this looks good.
The approach is slightly different from how I expected you to implement this.
Note that I don't know if it's actually better, it's more of a "if I had to
implement this, that's how I would have started." You've been working on this
for a bit, so I'm trusting your judgement.
Anyway, what I had in mind when I read your RFC and was doing the
RequestHandler work, triggered by a question from @labath, is that instead of
having one RequestHandler instance per command (which is what we have today),
we could instantiate a RequestHandler per incoming command (or a new object
that has a reference to the request hander and stores the arguments).
The read thread could decode its arguments and store away the protocol in the
RequestHandler/new object and add it to the queue. Beyond that, things work
pretty much how you've implemented them here, but with the logic that checks if
the request has been cancelled in the RequestHandler. More concretely the
operator doesn't take arguments anymore, and it's implemented something like
this:
```
void operator() {
if (IsCancelled()) // Check if our request ID is in the cancelled request set.
RespondCancelled();
llvm::Expected<Body> body = Run();
if (!body)
// Handle error
if (IsInterrupted()) // Check if the SBDebugger was interrupted.
RespondCancelled();
}
```
If we go with the new object (`PendingRequest`?) the request handlers can
remain what they are today, or if we want to store the data in them we'll need
to find a way to dispatch to them (similar to my comment in yesterday's PR).
Anyway, like I said, not sure if this is actually better, just sharing my
thoughts.
https://github.com/llvm/llvm-project/pull/130169
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
