3 ms·
Pretty good. Some opinions that fall under "be consistent within our code base" (which is why Uber should have a style guide at all), so all good. Just one com
by knorker 7y ago
Pretty good. Some opinions that fall under "be consistent within our code base" (which is why Uber should have a style guide at all), so all good.
Just one comment though:
> Embedding sync.Mutex
Never do this on an exported type. If you embed sync.Mutex that makes "Lock" and "Unlock" part of your exported interface. Now the caller of your API doesn't know if they are supposed to call Lock/Unlock, or if this is a pure internal implementation detail.
your `type Stats` "isn't" a mutex. It has a mutex.
- meddlepal 7y ago> Never do this on an exported type. If you embed sync.Mutex that makes "Lock" and "Unlock" part of your exported interface. Now the caller of your API doesn't know if they are supposed to call Lock/Unlock, or if this is a pure internal implementation detail. Isn't that exactly what they say? > Embed for private types or types that need to implement the Mutex interface. Personally, I would never embed a sync.Mutex though because even for an internal type it is confusing.
- knorker 7y agoCurious. Under "Returning Slices and Maps" they use it in their "Good" example.
- ArtWomb 7y agoAgree, but I can't recall ever seeing a package in the wild that labels itself as "concurrency-safe" and yet requires the caller to explicitly call Lock! HashMap is probably a good example https://github.com/cornelk/hashmap https://github.com/cornelk/hashmap