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

Added IEnumerable<T> interface to NativeHashSet #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sarkahn
Copy link

@sarkahn sarkahn commented Feb 5, 2020

No description provided.

-Changed ToNativeArray to use enumerator to avoid code duplication.
-Change ToList to use enumerator.
Copy link
Owner

@jacksondunstan jacksondunstan left a comment

Choose a reason for hiding this comment

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

@sarkahn Thanks for submitting this PR! I agree that implementinng IEnumerable<T> in NativeHashSet<T> is an improvement. I'd just like to see some changes before merging:

  • This project supports Unity 2018.1 which didn't yet have non-experimental support for C# 6 and its expression-bodied members. To maintain this support, please implement these as regular methods.
  • Please conform to the codebase's style by always adding curly braces to blocks, only using a single consecutive blank line, avoiding var, always adding access specifiers, putting curly braces on their own lines, omitting blank lines before the end of a block, and naming fields like m_FieldName.
  • Please remove all commented-out code
  • Please add unit tests to thoroughly exercise all functionality you added
  • Please make NativeHashSet<T>.Enumerator match NativeLinkedList<T>.Enumerator, such as by implementing IEquatable<Enumerator>, not performing safety checks in the constructor, keeping a copy of the whole NativeHashSet<T> rather than its contents.
  • Please add xml-doc to all struct contents: fields, methods, properties, other structs, etc. For methods and properties, include the safety requirements (e.g. read or write) and the runtime complexity (e.g. O(n)).
  • Please only perform safety checks when necessary (e.g. not in the Enumerator constructor)
  • Please replace the second if in Enumerator.MoveNext, which is redundant, with an else
  • Please reuse NativeMultiHashSetIterator in Enumerator to reduce duplication

I'll be happy to take another look once these changes are in. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants