feat: add locations request and references to variable types #494
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
debugAdapterProtocol.json
Outdated
| "type": "string", | ||
| "description": "A memory reference to a location appropriate for this result.\nFor pointer type eval results, this is generally a reference to the memory address contained in the pointer.\nThis attribute may be returned by a debug adapter if corresponding capability `supportsMemoryReferences` is true." | ||
| }, | ||
| "locationReference": { |
There was a problem hiding this comment.
Personally, I prefer the more concise version in the case where the thing it's referring to is unambiguous. It might actually be good that the Variable type is different because it does require extra attention to treat the value separately.
There was a problem hiding this comment.
In that case, maybe use the concise version in all the cases where the concise version is unambiguous (line 2251, 2673) so that the only place it is differentiated is in Variable?
There was a problem hiding this comment.
I would also be in favor of using valueLocationReference consistently. In particular, to ensure future extensibility.
In this particular place in the source code (i.e. for EvaluateResponse), it might even make sense to also have a declarationLocation (see #494 (comment)). I.e., I would retract my original comment
Afaict, there is no use case for declarationLocation inside EvaluateResponse, or am I missing something? Similar for OutputEvent
already after only 5 days.
I did not think through the other cases, yet, and I can well imagine that there will be future use cases for a declarationLocation also in the other places.
Using location instead of valueLocation now, would make future evolution a bit awkward, since we might then end up with location and declarationLocation
There was a problem hiding this comment.
Thanks for the feedback, I can see it could make sense as a future extensibility point here.
I think in the case of OutputEvent, where there is no corresponding data that could be referenced as a "declaration", it would make sense to keep it as locationReference, what do you think?
There was a problem hiding this comment.
I don't think I have ever used a debug-adapter which supports variable references in the OutputEvent, so I don't know whether having a declarationLocation would be useful there
roblourens
approved these changes
Aug 13, 2024
vogelsgesang
added a commit
to vogelsgesang/llvm-project
that referenced
this pull request
Aug 16, 2024
This commit implements support for the "declaration location" recently added by microsoft/debug-adapter-protocol#494 to the debug adapter protocol. For the `declarationLocationReference` we need a variable ID similar to the the `variablesReference`. I decided to simply reuse the `variablesReference` here and renamed `Variables::expandable_variables` and friends accordingly. Given that almost all variables have a declaration location, we now assign those variable ids to all variables. While `declarationLocationReference` effectively supersedes `$__lldb_extensions.declaration`, I did not remove this extension, yet, since I assume that there are some closed-source extensions which rely on it. I tested this against VS-Code Insiders. However, VS-Code Insiders currently only supports `valueLoctionReference` and not `declarationLocationReference`, yet. Locally, I hence tried to publish the declaration locations as value locations. However, it seems that VS-Code still has issues with correctly resolving file paths, as reported in microsoft/vscode#225546 (comment)
vogelsgesang
added a commit
to vogelsgesang/llvm-project
that referenced
this pull request
Aug 16, 2024
This commit implements support for the "declaration location" recently added by microsoft/debug-adapter-protocol#494 to the debug adapter protocol. For the `declarationLocationReference` we need a variable ID similar to the the `variablesReference`. I decided to simply reuse the `variablesReference` here and renamed `Variables::expandable_variables` and friends accordingly. Given that almost all variables have a declaration location, we now assign those variable ids to all variables. While `declarationLocationReference` effectively supersedes `$__lldb_extensions.declaration`, I did not remove this extension, yet, since I assume that there are some closed-source extensions which rely on it. I tested this against VS-Code Insiders. However, VS-Code Insiders currently only supports `valueLoctionReference` and not `declarationLocationReference`, yet. Locally, I hence tried to publish the declaration locations as value locations. However, it seems that VS-Code still has issues with correctly resolving file paths, as reported in microsoft/vscode#225546 (comment)
vogelsgesang
added a commit
to vogelsgesang/llvm-project
that referenced
this pull request
Aug 16, 2024
This commit implements support for the "declaration location" recently added by microsoft/debug-adapter-protocol#494 to the debug adapter protocol. For the `declarationLocationReference` we need a variable ID similar to the the `variablesReference`. I decided to simply reuse the `variablesReference` here and renamed `Variables::expandable_variables` and friends accordingly. Given that almost all variables have a declaration location, we now assign those variable ids to all variables. While `declarationLocationReference` effectively supersedes `$__lldb_extensions.declaration`, I did not remove this extension, yet, since I assume that there are some closed-source extensions which rely on it. I tested this against VS-Code Insiders. However, VS-Code Insiders currently only supports `valueLoctionReference` and not `declarationLocationReference`, yet. Locally, I hence tried to publish the declaration locations as value locations. However, it seems that VS-Code still has issues with correctly resolving file paths, as reported in microsoft/vscode#225546 (comment)
vogelsgesang
added a commit
to vogelsgesang/llvm-project
that referenced
this pull request
Aug 21, 2024
This commit implements support for the "declaration location" recently added by microsoft/debug-adapter-protocol#494 to the debug adapter protocol. For the `declarationLocationReference` we need a variable ID similar to the the `variablesReference`. I decided to simply reuse the `variablesReference` here and renamed `Variables::expandable_variables` and friends accordingly. Given that almost all variables have a declaration location, we now assign those variable ids to all variables. While `declarationLocationReference` effectively supersedes `$__lldb_extensions.declaration`, I did not remove this extension, yet, since I assume that there are some closed-source extensions which rely on it. I tested this against VS-Code Insiders. However, VS-Code Insiders currently only supports `valueLoctionReference` and not `declarationLocationReference`, yet. Locally, I hence published the declaration locations as value locations, and VS Code Insiders navigated to the expected places. Looking forward to proper VS Code support for `declarationLocationReference`.
vogelsgesang
added a commit
to vogelsgesang/llvm-project
that referenced
this pull request
Sep 16, 2024
This commit implements support for the "declaration location" recently added by microsoft/debug-adapter-protocol#494 to the debug adapter protocol. For the `declarationLocationReference` we need a variable ID similar to the the `variablesReference`. I decided to simply reuse the `variablesReference` here and renamed `Variables::expandable_variables` and friends accordingly. Given that almost all variables have a declaration location, we now assign those variable ids to all variables. While `declarationLocationReference` effectively supersedes `$__lldb_extensions.declaration`, I did not remove this extension, yet, since I assume that there are some closed-source extensions which rely on it. I tested this against VS-Code Insiders. However, VS-Code Insiders currently only supports `valueLoctionReference` and not `declarationLocationReference`, yet. Locally, I hence published the declaration locations as value locations, and VS Code Insiders navigated to the expected places. Looking forward to proper VS Code support for `declarationLocationReference`.
vogelsgesang
added a commit
to llvm/llvm-project
that referenced
this pull request
Sep 17, 2024
This commit implements support for the "declaration location" recently added by microsoft/debug-adapter-protocol#494 to the debug adapter protocol. For the `declarationLocationReference` we need a variable ID similar to the `variablesReference`. I decided to simply reuse the `variablesReference` here and renamed `Variables::expandable_variables` and friends accordingly. Given that almost all variables have a declaration location, we now assign those variable ids to all variables. While `declarationLocationReference` effectively supersedes `$__lldb_extensions.declaration`, I did not remove this extension, yet, since I assume that there are some closed-source extensions which rely on it. I tested this against VS-Code Insiders. However, VS-Code Insiders currently only supports `valueLoctionReference` and not `declarationLocationReference`, yet. Locally, I hence published the declaration locations as value locations, and VS Code Insiders navigated to the expected places. Looking forward to proper VS Code support for `declarationLocationReference`.
tmsri
pushed a commit
to tmsri/llvm-project
that referenced
this pull request
Sep 19, 2024
This commit implements support for the "declaration location" recently added by microsoft/debug-adapter-protocol#494 to the debug adapter protocol. For the `declarationLocationReference` we need a variable ID similar to the `variablesReference`. I decided to simply reuse the `variablesReference` here and renamed `Variables::expandable_variables` and friends accordingly. Given that almost all variables have a declaration location, we now assign those variable ids to all variables. While `declarationLocationReference` effectively supersedes `$__lldb_extensions.declaration`, I did not remove this extension, yet, since I assume that there are some closed-source extensions which rely on it. I tested this against VS-Code Insiders. However, VS-Code Insiders currently only supports `valueLoctionReference` and not `declarationLocationReference`, yet. Locally, I hence published the declaration locations as value locations, and VS Code Insiders navigated to the expected places. Looking forward to proper VS Code support for `declarationLocationReference`.
JDevlieghere
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Oct 16, 2024
This commit implements support for the "declaration location" recently added by microsoft/debug-adapter-protocol#494 to the debug adapter protocol. For the `declarationLocationReference` we need a variable ID similar to the `variablesReference`. I decided to simply reuse the `variablesReference` here and renamed `Variables::expandable_variables` and friends accordingly. Given that almost all variables have a declaration location, we now assign those variable ids to all variables. While `declarationLocationReference` effectively supersedes `$__lldb_extensions.declaration`, I did not remove this extension, yet, since I assume that there are some closed-source extensions which rely on it. I tested this against VS-Code Insiders. However, VS-Code Insiders currently only supports `valueLoctionReference` and not `declarationLocationReference`, yet. Locally, I hence published the declaration locations as value locations, and VS Code Insiders navigated to the expected places. Looking forward to proper VS Code support for `declarationLocationReference`. (cherry picked from commit 0cc2cd7)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #343