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

Simplify ForAttributeWithMetadataName #72979

Merged
merged 2 commits into from
Apr 12, 2024

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Apr 11, 2024

Discussions with @sharwell have indicated that thsi code is not necessary. Specifically, consider the semantics of what 'Collect' does. It takes in the N values from an IncrementalValuesProvider (note Values not Value), and produces an IncrementalValueProvider containing an ImmutableArray<> comprised of those individual values.

Now, consider an IncrementalValuesProvider<T> P1 which produces an IncrementalValueProvider<ImmutableArray<T>> P2 by doing P1.Collect(). If P1 produces the same values as its prior run, then that will be detected by the engine, which will cut off further processing, keeping P2 with the same array as hte prior run.

If, however, P1 does change in any way (according to its comparer) then we necessarily must get a new array in P2 containing those values. There is no need or value here for P2 to have any sort of array-comparer as we actually want array-identity to determine if P2 is creating new outputs or not.

--

Consider an example. Say that P1 produced "1", "2", "3", and thus P2 is ["1", "2", "3"]. If on rerun, P1 produces the same values (by equality, not necessarily identity), then we will get the same array, which will already behave properly with reference equality semantics. If we change to "1", "2", "4" though, and we produce ["1", "2", "4"], it does no good to do sequence equality for these arrays. They will be different (as otherwise we wouldn't generate the ImmutableArray in the first place). As such, doing a reference-equality check is actually preferred as it's O(1) instead of O(n).

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner April 11, 2024 05:41
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 11, 2024
@AlekseyTs
Copy link
Contributor

@chsienki Please review

@CyrusNajmabadi CyrusNajmabadi merged commit 9f22861 into dotnet:main Apr 12, 2024
24 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the cleanupCollect branch April 12, 2024 21:56
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 12, 2024
@dibarbet dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024
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.

6 participants