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.

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

Flatten SourceMethodSymbol hierarchy #76107

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

333fred
Copy link
Member

@333fred 333fred commented Nov 26, 2024

Once upon a time, there were source method symbols that didn't have attributes, and thus we had SourceMethodSymbol and SourceMethodSymbolWithAttributes, to simplify/save on symbol size for such methods. Today, there is no such difference, and the only direct implementor of SourceMethodSymbol is SourceMethodSymbolWithAttributes. Therefore, I've cleaned up the hierarchy by removing the dead intermediate class. Highly suggest reviewing commit-by-commit, so that the rename-with-no-changes is correctly displayed. I will not be squash merging this PR to preserve that rename in history as well.

@333fred 333fred requested a review from a team as a code owner November 26, 2024 22:02
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 26, 2024
@@ -1746,5 +1746,78 @@ internal override System.Reflection.MethodImplAttributes ImplementationAttribute

internal override int TryGetOverloadResolutionPriority()
=> GetEarlyDecodedWellKnownAttributeData()?.OverloadResolutionPriority ?? 0;

Copy link
Member Author

Choose a reason for hiding this comment

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

These methods were copy/pasted from SourceMethodSymbol.cs with no changes to their content.

@jcouv jcouv self-assigned this Nov 26, 2024
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 3)

Once upon a time, there were source method symbols that didn't have attributes, and thus we had `SourceMethodSymbol` and `SourceMethodSymbolWithAttributes`, to simplify/save on symbol size for such methods. Today, there is no such difference, and the only direct implementor of `SourceMethodSymbol` is `SourceMethodSymbolWithAttributes`. Therefore, I've cleaned up the hierarchy by removing the dead intermediate class.
@333fred
Copy link
Member Author

333fred commented Nov 27, 2024

@cston @jcouv, I've taken advice from Aleksey and restructured the approach to keep everything in the same location; instead, I now just make SourceMethodSymbol a partial type and leave the existing files alone. Apologies for the force push, but it was the cleanest way to approach the restructure. The upside is that the change is now 11 lines long, so it's not very hard to review.

@333fred 333fred enabled auto-merge (squash) November 27, 2024 18:14
@333fred 333fred merged commit 6cc106c into dotnet:main Nov 27, 2024
24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 27, 2024
Copy link
Member

@jjonescz jjonescz Nov 28, 2024

Choose a reason for hiding this comment

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

Should we also rename the file? Perhaps to SourceMethodSymbol_Attributes.cs

Copy link
Member Author

Choose a reason for hiding this comment

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

The goal with this version was to reduce noise in git history, so I didn't.

Copy link
Member

Choose a reason for hiding this comment

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

Git would likely detect it as rename, so there would be no noise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants