Move merging of partial members to an earlier phase of members construction. #76871
Conversation
…uction. Fixes dotnet#76651. Fixes dotnet#75002. Related to dotnet#76842. Related to dotnet#76870.
dotnet-issue-labeler
bot
added
the
untriaged
|
@RikkiGibson, @cston, @dotnet/roslyn-compiler Please review |
|
@jjonescz, FYI |
| } | ||
| } | ||
|
|
||
| void mergePartialMethods(ArrayBuilder<Symbol> nonTypeMembers, SourceOrdinaryMethodSymbol currentMethod, SourceOrdinaryMethodSymbol prevMethod, BindingDiagnosticBag diagnostics) |
There was a problem hiding this comment.
static?
I do not think I changed anything that affects ability to place the modifier on the method. Therefore, I won't try adding it, unless there is other feedback to take care of.
There was a problem hiding this comment.
Scratch that, it looks like I removed DuplicateMembersByNameIfCached call
There was a problem hiding this comment.
It looks like the containing method MergePartialMembers could be marked static too
| } | ||
| } | ||
|
|
||
| void mergePartialProperties(ref Dictionary<ReadOnlyMemory<char>, ImmutableArray<Symbol>> membersByName, ReadOnlyMemory<char> name, SourcePropertySymbol currentProperty, SourcePropertySymbol prevProperty, BindingDiagnosticBag diagnostics) | ||
| void mergePartialProperties(ArrayBuilder<Symbol> nonTypeMembers, SourcePropertySymbol currentProperty, SourcePropertySymbol prevProperty, BindingDiagnosticBag diagnostics) |
There was a problem hiding this comment.
static?
Same response
|
@RikkiGibson, @cston, @dotnet/roslyn-compiler For the second review |
1 similar comment
|
@RikkiGibson, @cston, @dotnet/roslyn-compiler For the second review |
| } | ||
|
|
||
| private static ImmutableArray<Symbol> Remove(ImmutableArray<Symbol> symbols, Symbol symbol) | ||
| private static void Remove(ArrayBuilder<Symbol> symbols, Symbol symbol) |
There was a problem hiding this comment.
It looks like we call Remove once for each partial method or property that has both parts. Does the symbols array contain all non-type members? If that's the case, the number of ReferenceEquals() calls across all Remove calls for the containing type will be roughly N*M where N is the number of non-type members and M is the number of partial members. Are there cases where that might be an issue?
There was a problem hiding this comment.
Are there cases where that might be an issue?
It is hard to tell for sure. I assume that ReferenceEquals call is pretty fast, since it is optimized away and probably roughly close to a pointer comparison. Also, M is likely not very big and no more than N/2. Also, on average, the match will be found before we check all N items, and each removal decreases N by one.
There was a problem hiding this comment.
The M that I am using is not the number of partial members. It is the number of partial members that we are going to remove, which is no more than the number of partial members divided by two.
src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Revert the rename.
This reverts commit e3d2282.
Fixes #76651.
Fixes #75002.
Related to #76842.
Related to #76870.