Skip to content

Conversation

@CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Oct 14, 2025

Fixes #29707

This is faster in C# for some symbols as it does not need to realize arrays to wrap a single location for many symbols. There is also GetFirstLocationOrNone if you need to call on a symbol which does not have locations.

CustomSymbolDisplayFormatter.QualifiedName(outermostVariantInterface),
AssociatedSymbol.Name),
Locations(0)))
GetFirstLocation()))
Copy link
Member Author

Choose a reason for hiding this comment

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

note: if we don't want to update VB, i'm fine with that and can revert this. But it seems sensible to me. It also means we can optimize GetFirstLocation in the future (like we have for C#) and get the benfit for vb as well automatically.

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review October 14, 2025 16:58
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner October 14, 2025 16:58
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

I'm fine with adding this to VB, but only if it doesn't stackoverflow

@AlekseyTs
Copy link
Contributor

@CyrusNajmabadi Please use more descriptive PR title so that it would be clear what is the goal and expected impact of this change.

@CyrusNajmabadi CyrusNajmabadi changed the title Use GetFirstLocation Update code to use GetFirstLocation() instead of Locations[0] or Locations.First() Oct 14, 2025
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 (commit 11)

@jcouv jcouv self-assigned this Oct 15, 2025
@CyrusNajmabadi CyrusNajmabadi requested a review from jcouv October 15, 2025 17:16
@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler for a second pair of eyes.

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 (commit 12)

@CyrusNajmabadi CyrusNajmabadi merged commit 3a723bb into dotnet:main Oct 20, 2025
28 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the firstLocation branch October 20, 2025 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace uses of Locations[0] and Locations.First()

4 participants