-
Notifications
You must be signed in to change notification settings - Fork 65
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
Fix value conversion for CONFIG GET
.
#2381
Fix value conversion for CONFIG GET
.
#2381
Conversation
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.
A few small suggestions, but seems good otherwise.
// Second parameter is a function which returns true if value needs to be converted | ||
SingleOrMultiNode( | ||
&'a Option<ExpectedReturnType<'a>>, | ||
Option<fn(Value) -> 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.
Not sure how difficult it would be to change, but generally you should prefer taking a generic implementing a closure trait like F: Fn(Value) -> bool
if possible because plain function pointers can't capture their environment like closures can. We don't really need the extra flexibility for our existing cases, so this isn't critical to do. Might not be worth making this change if it involves an extra Box
alloc, but maybe worth a try at least.
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.
Do you mean something like this?
pub(crate) enum ExpectedReturnType<'a, F: Fn(Value) -> bool> {
Map {
key_type: &'a Option<ExpectedReturnType<'a, F>>,
value_type: &'a Option<ExpectedReturnType<'a, F>>,
},
// Second parameter is a function which returns true if value needs to be converted
SingleOrMultiNode(
&'a Option<ExpectedReturnType<'a, F>>,
Option<F>,
),
...
(I also need to update all references to ExpectedReturnType
type)
Or
pub(crate) enum ExpectedReturnType<'a> {
Map {
key_type: &'a Option<ExpectedReturnType<'a>>,
value_type: &'a Option<ExpectedReturnType<'a>>,
},
// Second parameter is a function which returns true if value needs to be converted
SingleOrMultiNode(
&'a Option<ExpectedReturnType<'a>>,
Option<&'a dyn Fn(Value) -> 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.
I was thinking something more like the latter. I was actually expecting we'd take Box<dyn fn(Value) -> bool
so we could own the closure, but we could probably get away with the reference instead. I'm aware this uses dynamic dispatch, but I think the performance hit is fairly negligible here.
e51ad3c
to
507f5f9
Compare
Signed-off-by: Yury-Fridlyand <[email protected]>
507f5f9
to
0cbbde3
Compare
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Multi-node RESP2 value wasn't converted
Fixes #2392