Code and style improvements #106
Merged
Conversation
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
updated docstrings describing Set interface methods to start with a method name for a correct documentation generation (and avoiding tons of warnings in goland)
now constructors returns by value instead of by reference a newly constructed sets
renamed method receivers to be a single-letter equal to a first letter in object's name, as community recommends
`threadUnsafeSet` is already a map that is a reference-type, so passing it by-value still lets us mutate an origin object, but decreases a number of pointers, which in turn, decreases a GC-pressure, (potentially) makes less useless operations and (definitely) makes code better to read/understand.
Another point of the change is to a bit "upgrade" a `Clear()` method. In the old implementation, it simply constructed a new `threadUnsafeSet` and did mutate an origin pointer to point at our new set. This commits an extra allocation (I would expect a `Clear()` method to be allocations-free), and fallbacks already allocated map's size to the default one. So I replaced this with clearing itself using `mapclear()` function (that implicitly replaces `for key := range d { delete(d, key) }`). This is actually pretty expensive operation, but does no allocations, that is maybe even cheaper than vice-versa. Anyway user always can construct a new instance of the set by itself, but cannot clear underlying map manually in case he really needs it - to decrease amount of memory used in average, number of which is actually affected a lot by internally allocating a new map
|
@fakefloordiv - thanks for the changes I’ll be reviewing this in the coming days. Overall looks like some nice cleanup. |
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.
Set[T]interface methods to be compatible with a community styleguide - to start with a name of actual method. This also allows us to generate documentation more effectively (because go doc by default ignores docstrings that are not starting with an object name)Clear()method. It now REALLY clears the set instead of allocating a new one. All the details are provided in the description of the last one commit