-
Notifications
You must be signed in to change notification settings - Fork 230
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
What: add spec for array ops with []uint8 or []byte type #128
Conversation
Hi @mcspring You make a really good point about the array ops and thanks for the PR (with tests!) - there's a conflict on the branch though but should be good to merge after. Dom |
@@ -60,6 +60,15 @@ var ( | |||
typeTimeDuration = reflect.TypeOf(time.Duration(0)) | |||
) | |||
|
|||
var ( | |||
// spec for []uint8 or []byte encoding | |||
arrayOps = map[string]bool{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nitpick here ( sorry! )
IIRC map[string]struct{}
could be used here, and that one has an optimised lookup code when you want to only check for the presence of an element.
this would transform the map lookups to
if _, ok := arrayOps[name]; ok {
[...]
}
Which is not as clean. I'll let you make a call here. I haven't benchmarked the code, so I can't tell how much faster it is, but it is the idiomatic go way of doing things FWIW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way will cause one more alloc, which can be avoid from previous.
Why: * mongodb aquires array for $in/$nin/$all ops How: * encode with array for spec
@domodwyer synced yet. |
Looks good to me - thanks @mcspring Dom |
Why:
How: