Added IEnumerable<T> interface to NativeHashSet#7
Open
sarkahn wants to merge 2 commits intojacksondunstan:masterfrom
Open
Added IEnumerable<T> interface to NativeHashSet#7sarkahn wants to merge 2 commits intojacksondunstan:masterfrom
sarkahn wants to merge 2 commits intojacksondunstan:masterfrom
Conversation
-Changed ToNativeArray to use enumerator to avoid code duplication. -Change ToList to use enumerator.
jacksondunstan
requested changes
Feb 21, 2020
Owner
jacksondunstan
left a comment
There was a problem hiding this comment.
@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 likem_FieldName. - Please remove all commented-out code
- Please add unit tests to thoroughly exercise all functionality you added
- Please make
NativeHashSet<T>.EnumeratormatchNativeLinkedList<T>.Enumerator, such as by implementingIEquatable<Enumerator>, not performing safety checks in the constructor, keeping a copy of the wholeNativeHashSet<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
Enumeratorconstructor) - Please replace the second
ifinEnumerator.MoveNext, which is redundant, with anelse - Please reuse
NativeMultiHashSetIteratorinEnumeratorto reduce duplication
I'll be happy to take another look once these changes are in. Thanks again!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.