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

Clean up HashSet.cs copy #7340

Open
danmoseley opened this issue Jan 26, 2022 · 2 comments
Open

Clean up HashSet.cs copy #7340

danmoseley opened this issue Jan 26, 2022 · 2 comments
Labels
Milestone

Comments

@danmoseley
Copy link
Member

danmoseley commented Jan 26, 2022

We have a partial copy of HashSet as it existed in the .NET Framework circa 2008 or something, with the following comment:

==================================================================================================================
MSBUILD COMMENT:
Ripped off from Hashset.cs with the following changes:
* class renamed
* unnecessary methods and attributes if-deffed out (code retained to help windiff, but indented)
* require T implements IKeyed, and accept IKeyed directly where necessary
* all constructors require a comparer -- an IEqualityComparer<IKeyed> -- to avoid mistakes
* change Contains to give you back the found entry, rather than a boolean
* change Add so that it always adds, even if there's an entry already present with the same name.
We want "replacement" semantics, like a dictionary keyed on name.
* constructor that allows the collection to be read-only
* implement IDictionary<string, T>
* some convenience methods taking 'string' as overloads of methods taking IKeyed
Other than this it is modified absolutely minimally to make it easy to diff with the originals (in the Originals folder)
to verify that no errors were introduced, and make it easier to possibly pick up any future bug fixes to the original.
The care taken to minimally modify this means that it is not necessary to carefully code review this complex class,
nor unit test it directly.
==================================================================================================================

The rationale for this odd pattern of fragmented dead code was to diff for future updates to HashSet -- but the live copy of HashSet here has since changed so substantially that it can likely hardly be diffed at all.

As a matter of healthy cleanup, I suggest to delete all the dead code.

There have also been several improvements to HashSet since the copy was done, for perf reasons - either obvious ones or to produce better codegen. It would be nice to copy them (again, without bringing dead code) for some small perf wins. I expect this will be fairly easy to do by eyeballing them side by side.

@danmoseley danmoseley added the needs-triage Have yet to determine what bucket this goes in. label Jan 26, 2022
@elachlan
Copy link
Contributor

I have been attempting this. The CORE version was re-written in June of 2020. I think it might be best to take that version and refactor it with the changes from MSBUILD.

I will let you know how I go.

@rainersigwald
Copy link
Member

I wouldn't spend a lot of time on this: we don't have any indication that the current implementation is causing a bottleneck today, and changes are likely to be hard to review due to the overall complexity of the class.

@benvillalobos benvillalobos removed the needs-triage Have yet to determine what bucket this goes in. label Jan 27, 2022
@benvillalobos benvillalobos added this to the Backlog milestone Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants