[lldb-dap] Refactor request handlers (NFC) #128262
Conversation
JDevlieghere
force-pushed
the
request
branch
from
03bf2fa to
e6cbcc3
Compare
| void SetSourceMapFromArguments(const llvm::json::Object &arguments); | ||
|
|
||
| protected: | ||
| DAP &dap; |
There was a problem hiding this comment.
I've been thinking about the naming convention of the DAP object and I wonder if we should refer to this DAP as a session?
e.g. DAP &session;
That kind of represents what this is actually managing in the context of lldb-dap. The DAP object is the debug session and is coordinating the dap.debugger / dap.target with the various requests from the client.
I'm open to other ideas, but its something I've noticed while working on lldb-dap code.
There was a problem hiding this comment.
Session sounds good. Context would probably be another good option?
There was a problem hiding this comment.
Context would also work, either it helps with clarity since dap is a bit vague, but this could also be done in a follow up.
| enum LaunchMethod { Launch, Attach, AttachForSuspendedLaunch }; | ||
|
|
||
| void SendProcessEvent(LaunchMethod launch_method); | ||
| void SetSourceMapFromArguments(const llvm::json::Object &arguments); |
There was a problem hiding this comment.
Whats the reason to have these here vs in a specific Request handler below?
Are these shared helpers between different requests?
There was a problem hiding this comment.
They are indeed shared, but not by many request handlers. I don't think this is a good place for them, but I'm trtying to minimize the changes.
| enum LaunchMethod { Launch, Attach, AttachForSuspendedLaunch }; | ||
|
|
||
| void SendProcessEvent(LaunchMethod launch_method); | ||
| void SetSourceMapFromArguments(const llvm::json::Object &arguments); |
There was a problem hiding this comment.
I know this isn't part of your change specifically, but I do think it would be good for us to create richer types to represent some arguments like this arguments parameter.
That was the motivation for the creating the POD types in Protocol.h to have a clear boundary between protocol types, lldb debugger types (generally anything like lldb::SB*) and the helpers that are part of lldb-dap used for coordinating between the two.
| public: | ||
| using Request::Request; | ||
| static llvm::StringLiteral getName() { return "attach"; } | ||
| void operator()(const llvm::json::Object &request) override; |
There was a problem hiding this comment.
This is showing my lack of c++ knowledge, but if we did create specific types for these handles, like a struct AttachArguments and struct AttachResponseBody can we override those here?
There was a problem hiding this comment.
Not directly, no, but you can do exactly what you did in your PR. Have this function take a const protocol::Request &req and then implement it int terms of types unique to the class. They become an implementation detail of the RequestHandler that nobody else needs to know about.
There was a problem hiding this comment.
Thats a good point, maybe with this structure my other PR will be easier for us to add.
I wonder, can the subclass do something like:
class AttachHandler: RequestHandler<AttachArguments, AttachBodyResponse> {
...
Then we could use the protocol types for serialization. I might look into that as a follow up.
| // Both attach and launch take a either a sourcePath or sourceMap | ||
| // argument (or neither), from which we need to set the target.source-map. | ||
| void Request::SetSourceMapFromArguments(const llvm::json::Object &arguments) { |
There was a problem hiding this comment.
I think the fact this is present in both the launch and attach events means we may want to move this kind of logic to the DAP object or to some other helper.
I think we'd end up with a lot of various helpers in the base class, which may not be applicable to the various subclasses.
There was a problem hiding this comment.
Agreed. Are you okay with doing that in a separate PR? If I move stuff into the base class, it becomes obvious what needs to be moved (I'll add a FIXME).
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
JDevlieghere
force-pushed
the
request
branch
from
e2728b6 to
feae281
Compare
Conflicts: lldb/tools/lldb-dap/lldb-dap.cpp
|
I'm marking this as ready for review. I think the 9 requests that have been migrated are pretty representative and I intentionally made it so that the old and new way of register requests keeps working. I was running the tests between every PR. However, resolving the merge conflict with main made me realize that almost anything touching |
lldb/tools/lldb-dap/lldb-dap.cpp
Outdated
| continue; | ||
| // Both attach and launch take a either a sourcePath or sourceMap | ||
| // argument (or neither), from which we need to set the target.source-map. | ||
| void SetSourceMapFromArguments(DAP &dap, const llvm::json::Object &arguments) { |
There was a problem hiding this comment.
Is this repeated? Or I think maybe this is still here for the existing request_launch function that hasn't moved over yet.
Should this move to the a RequestHelpers.h like we have for EventHelpers.h?
There was a problem hiding this comment.
Yeah, I managed to avoid dit by migrating to LaunchRequestHandler & RestartRequestHandler.
JDevlieghere
changed the title
Continuation of the work started in llvm#128262.
There was a problem hiding this comment.
I am thinking about how would this new approach work if we started executing requests in parallel (I know we're not planning to do that now, but this sort of started out from that), and it seems to me that the this design (a "singleton" object with a non-const member) isn't the best design for that. I think this could be improved in at least two ways, but the choice depends on which way we'd like to take this:
- option one is to make operator()
const. This guarantees the implementation doesn't store any information about the currently processed request in the object itself. This prevents "leaking" of data from one request into another and as a consequence, makes the object safe to use concurrently - option two is to create a new handler object for request. This takes the opposite approach and actually encourages to (transiently) store information about the current request in the handler object (but then makes it safe by making sure the information is compartmentalized)
I don't want to make a choice here, since it depends on where you (the people who are actually going to be working on this) want to take this, but I think it would good to make a choice here, even if it gets changed later. The first approach (make the method const) would be most similar to status quo (because a free function doesn't even have an object to store information to), so maybe that could be the default?
Currently, all request handlers are implemented as free functions in lldb-dap.cpp. That file has grown to over 5000 lines and is starting to become hard to maintain. This PR moves the request handlers into their own class (and file), together with their documentation. This PR migrates about a third of the request handlers and the rest will be migrated in subsequent commits. I'm merging this in an incomplete state because almost any lldb-dap change is going to result in merge conflicts and migrating request handlers one by one is easier to review.
) Continuation of the work started in llvm#128262.
…lvm#128549) Continuation of the work started in llvm#128262. Builds on top of llvm#128453.
…28550) Continuation of the work started in llvm#128262. Builds on top of llvm#128549.
… handlers (llvm#128551) Continuation of the work started in llvm#128262. Builds on top of llvm#128550.
Completes the work started in llvm#128262. This PR removes the old way of register request handlers with callbacks and makes the operator const.
Currently, all request handlers are implemented as free functions in lldb-dap.cpp. That file has grown to over 5000 lines and is starting to become hard to maintain. This PR moves the request handlers into their own class (and file), together with their documentation. This PR migrates about a third of the request handlers and the rest will be migrated in subsequent commits. I'm merging this in an incomplete state because almost any lldb-dap change is going to result in merge conflicts and migrating request handlers one by one is easier to review.
) Continuation of the work started in llvm#128262.
…lvm#128549) Continuation of the work started in llvm#128262. Builds on top of llvm#128453.
…28550) Continuation of the work started in llvm#128262. Builds on top of llvm#128549.
… handlers (llvm#128551) Continuation of the work started in llvm#128262. Builds on top of llvm#128550.
Completes the work started in llvm#128262. This PR removes the old way of register request handlers with callbacks and makes the operator const.
Currently, all request handlers are implemented as free functions in lldb-dap.cpp. That file has grown to over 5000 lines and is starting to become hard to maintain. This PR moves the request handlers into their own class (and file), together with their documentation. This PR migrates about a third of the request handlers and the rest will be migrated in subsequent commits. I'm merging this in an incomplete state because almost any lldb-dap change is going to result in merge conflicts and migrating request handlers one by one is easier to review.
) Continuation of the work started in llvm#128262.
…lvm#128549) Continuation of the work started in llvm#128262. Builds on top of llvm#128453.
…28550) Continuation of the work started in llvm#128262. Builds on top of llvm#128549.
… handlers (llvm#128551) Continuation of the work started in llvm#128262. Builds on top of llvm#128550.
Completes the work started in llvm#128262. This PR removes the old way of register request handlers with callbacks and makes the operator const.
Currently, all request handlers are implemented as free functions in lldb-dap.cpp. That file has grown to over 5000 lines and is starting to become hard to maintain. This PR moves the request handlers into their own class (and file), together with their documentation. This PR migrates about a third of the request handlers and the rest will be migrated in subsequent commits. I'm merging this in an incomplete state because almost any lldb-dap change is going to result in merge conflicts and migrating request handlers one by one is easier to review.
) Continuation of the work started in llvm#128262.
…lvm#128549) Continuation of the work started in llvm#128262. Builds on top of llvm#128453.
…28550) Continuation of the work started in llvm#128262. Builds on top of llvm#128549.
… handlers (llvm#128551) Continuation of the work started in llvm#128262. Builds on top of llvm#128550.
Completes the work started in llvm#128262. This PR removes the old way of register request handlers with callbacks and makes the operator const.
Currently, all request handlers are implemented as free functions in lldb-dap.cpp. That file has grown to over 5000 lines and is starting to become hard to maintain. This PR moves the request handlers into their own class (and file), together with their documentation. This PR migrates about a third of the request handlers and the rest will be migrated in subsequent commits. I'm merging this in an incomplete state because almost any lldb-dap change is going to result in merge conflicts and migrating request handlers one by one is easier to review.
) Continuation of the work started in llvm#128262.
…lvm#128549) Continuation of the work started in llvm#128262. Builds on top of llvm#128453.
…28550) Continuation of the work started in llvm#128262. Builds on top of llvm#128549.
… handlers (llvm#128551) Continuation of the work started in llvm#128262. Builds on top of llvm#128550.
Completes the work started in llvm#128262. This PR removes the old way of register request handlers with callbacks and makes the operator const.
Currently, all request handlers are implemented as free functions in lldb-dap.cpp. That file has grown to over 5000 lines and is starting to become hard to maintain. This PR moves the request handlers into their own class (and file), together with their documentation.