-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Complete the Implementation of DAP modules explorer. #139934
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) ChangesWhat it now looks like. It is a good idea to remove the, symbol status and name from the tree children ?. Full diff: https://github.com/llvm/llvm-project/pull/139934.diff 4 Files Affected:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
lldb/tools/lldb-dap/JSONUtils.cpp
Outdated
std::string loaded_addr; | ||
llvm::raw_string_ostream os_hex(loaded_addr); | ||
os_hex << llvm::format_hex( | ||
module.GetObjectFileHeaderAddress().GetLoadAddress(target), | ||
sizeof(lldb::addr_t)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed the load address was a "number" but the spec says it's a "string" in which case I agree it makes more sense to the hex conversion here.
You can simplify this something like this:
object.try_emplace("addressRange", llvm::format({0:x}, module.GetObjectFileHeaderAddress().GetLoadAddress(target)).str());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
microsoft/debug-adapter-protocol#424 was filed to clarify how the address range is supposed to be represented. It hasn't had any movement recently, so we could ping the issue to see if the folks that work on the DAP have any more recent thoughts.
lldb/tools/lldb-dap/package.json
Outdated
"menus": { | ||
"view/item/context": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also disable the Copy Value
command from the command prompt?
adding:
"commandPalette": [{"command": "lldb-dap.modules.copyProperty", "when": "false"}],
Should hide it, since its not useful without the context of a tree view item.
This extends the TreeView to show the module property as a tree item instead of rendering it through the markdown tooltip. 
What it now looks like.
It is a good idea to remove the, symbol status and name from the tree children ?.