Skip to content
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.

Sign up for GitHub

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

Jump to bottom

EE: Support compact name in IDkmLanguageInstructionDecoder.GetMethodName() implementation #75764

Merged
merged 6 commits into from
Nov 15, 2024
Merged

EE: Support compact name in IDkmLanguageInstructionDecoder.GetMethodName() implementation #75764

merged 6 commits into from
Nov 15, 2024

Conversation

cston
Copy link
Member

@cston cston commented Nov 6, 2024 •
edited
Loading

The compact name is simply the method metadata name without arguments or type arguments, or for an accessor, the property or event name.

The table below includes differences between the strings returned from GetMethodName() with and without DkmVariableInfoFlags.CompactName.

Member with CompactName without
method M C.M
property accessor P C.P.get
indexer accessor this[] C.this[int].get
constructor C C.C
destructor ~C C.~C
unary operator operator + C.operator +
conversion operator implicit operator int C.implicit operator int
... ... ...

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Interactive untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 6, 2024
@cston
Copy link
Member Author

cston commented Nov 6, 2024

/azp run roslyn-CI

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cston cston marked this pull request as ready for review November 6, 2024 23:44
@cston cston requested a review from a team as a code owner November 6, 2024 23:44
@cston cston requested review from tmat and a team and removed request for a team November 6, 2024 23:44
jjonescz
jjonescz reviewed Nov 7, 2024
View reviewed changes
src/ExpressionEvaluator/Core/Source/ExpressionCompiler/LanguageInstructionDecoder.cs
@@ -38,16 +38,24 @@ string IDkmLanguageInstructionDecoder.GetMethodName(DkmLanguageInstructionAddres
// but it was ignored. Furthermore, it's not clear what FullNames would mean with respect
// to argument names in C# or Visual Basic. For consistency with the old behavior, we'll
// just ignore the flag as well.
Debug.Assert((argumentFlags & (DkmVariableInfoFlags.FullNames | DkmVariableInfoFlags.Names | DkmVariableInfoFlags.Types)) == argumentFlags,
Debug.Assert((argumentFlags & (DkmVariableInfoFlags.FullNames | DkmVariableInfoFlags.Names | DkmVariableInfoFlags.Types | DkmVariableInfoFlags.CompactName)) == argumentFlags,
Copy link
Member

@jjonescz jjonescz Nov 7, 2024 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious where will the CompactName be used exactly in VS/EE UI? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBD.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjonescz FWIW, here's the doc for the new flag:

  // Summary:
  //     If specified, the expression evaluator will create a very compact name for the
  //     frame. This is intended for UI presentation in cases where screen space is very
  //     limited. The suggested implementation is to return only the method name, with
  //     no namespace, class name, or generic arguments.

@cston
Copy link
Member Author

cston commented Nov 7, 2024

/azp run roslyn-CI

@cston cston requested a review from a team November 7, 2024 17:33
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cston cston marked this pull request as draft November 7, 2024 18:15
@cston cston marked this pull request as ready for review November 7, 2024 22:50
jjonescz
jjonescz reviewed Nov 8, 2024
View reviewed changes
src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/CSharpInstructionDecoder.cs Outdated
@@ -36,7 +36,8 @@ private CSharpInstructionDecoder()

internal override string GetCompactName(MethodSymbol method)
{
return (method.AssociatedSymbol ?? method).Name;
var symbol = method.AssociatedSymbol ?? method;
return symbol.ToDisplayString(CompactNameFormat);
Copy link
Member

@jjonescz jjonescz Nov 8, 2024 •
edited by cston
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this from Name to DisplayString? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're using ToDisplayString() so the member name in the string returned from IDkmLanguageInstructionDecoder.GetMethodName() is consistent regardless of whether DkmVariableInfoFlags.CompactName is used. See table in PR description.

@cston cston requested a review from a team November 12, 2024 17:10
@cston
Copy link
Member Author

cston commented Nov 13, 2024

@dotnet/roslyn-compiler for a second review, thanks.

@cston cston requested a review from a team November 13, 2024 16:42
@jcouv jcouv self-assigned this Nov 14, 2024
jcouv
jcouv reviewed Nov 14, 2024
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with review pass (iteration 2)

jcouv
jcouv reviewed Nov 14, 2024
View reviewed changes
src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/CSharpInstructionDecoder.cs Outdated
@@ -34,6 +34,18 @@ private CSharpInstructionDecoder()
AddMemberOptions(SymbolDisplayMemberOptions.IncludeParameters).
WithParameterOptions(SymbolDisplayParameterOptions.IncludeType);

internal static readonly SymbolDisplayFormat s_indexerCompactNameFormat = CompactNameFormat.
WithMemberOptions(SymbolDisplayMemberOptions.IncludeParameters);
Copy link
Member

@jcouv jcouv Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: may be worth a comment or making SymbolDisplayParameterOptions.None explicit for clarity #Closed

jcouv
jcouv approved these changes Nov 14, 2024
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 4)

@build-analysis build-analysis bot mentioned this pull request Nov 15, 2024
Open
3 tasks
@cston cston merged commit ad68d4b into dotnet:main Nov 15, 2024
28 checks passed
@cston cston deleted the CompactName branch November 15, 2024 02:00
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 15, 2024
This was referenced Nov 17, 2024
Closed
Closed
Closed
@jjonescz jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcouv jcouv jcouv approved these changes

@jjonescz jjonescz jjonescz approved these changes

@tmat tmat Awaiting requested review from tmat

Assignees

@jcouv jcouv

Labels
Area-Compilers Area-Interactive untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.13 P2
Development

Successfully merging this pull request may close these issues.
3 participants
@cston @jjonescz @jcouv