-
Notifications
You must be signed in to change notification settings - Fork 113
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
[Dynamic Selection] Customization - Design approach for free function that calls member if available or default other #1023
[Dynamic Selection] Customization - Design approach for free function that calls member if available or default other #1023
Conversation
AnuyaWelling2801
commented
Jul 21, 2023
- Added default free functions for invoke, invoke with a handle and invoke async
- Implemented customizations for invoke, invoke with a handle and invoke async, so that if a policy implemented any of these functions it would be redirected to the policy's member function.
- Removed the policy wrapper.
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.
Looks mostly ok, except for some forwarding that needs to be checked.
auto report(const Property &prop, const Value &value) const { | ||
return const_cast<const scoring_policy_t &>(*scoring_policy_).report(prop, value); | ||
else { | ||
return invoke_async(std::forward<DSPolicy>(dp), std::forward<DSPolicy>(dp).select(std::forward<Function>(f), std::forward<Args>(args)...), std::forward<Function>(f), std::forward<Args>(args)...); |
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.
You are forwarding both f and args twice in the same function. Might be ok, but you should be sure they can never be moved in a call to select.
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.
Fixed
auto get_wait_list(){ | ||
return scoring_policy_->get_wait_list(); | ||
else{ | ||
return wait(std::forward<DSPolicy>(dp).invoke_async(std::forward<DSPolicy>(dp).select(std::forward<Function>(f), std::forward<Args>(args)...), std::forward<Function>(f), std::forward<Args>(args)...)); |
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.
Same possible issue with double forwarding.
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.
Fixed
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.
Looks go to me