-
Notifications
You must be signed in to change notification settings - Fork 969
cln-plugin: add a hook builder to make use of before, after and filters #8704
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
base: master
Are you sure you want to change the base?
Conversation
|
ACK 9a6486e |
| self | ||
| } | ||
|
|
||
| pub fn hook_from_builder(mut self, hook: HookBuilder<S>) -> Builder<S, I, O> { |
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.
Is there a point in keeping the HookBuilder around? I quite like using mut HookBuilder as argument type, because it means we own the instance, and we can disect it however we like, so in this case I think we can skip the hook.name.clone() invokation and directly use hook.name, saving us a clone in normal operations 🤔
Very minor thing, but I was wondering what you think of such an optimization.
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.
Afaik we don't keep it around since hook.build() consumes it. Since we need the String in name here and to make the Hook in build() we need to clone() it somewhere, right?
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.
That's the thing that I'm wondering, if we consume the HookBuilder, the String is free to be reassigned, and the Builder could reuse the String. No idea if this is done by the compiler, and more of a curiosity to be honest, so this should absolutely not hold up progress. This can be merged as is.
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 about every clone alot aswell 😄 . self.hooks has the hook.name as key and in the value struct. Both are owned so we must either
- clone the name for the key or the value
- use borrowed objects (then we probably need to start using lifetimes, which seems not worth it here imo)
- refactor some stuff so we don't have the name in both key and value
|
It seems this one missed 25.12 because it wasn't linked to an issue :( added it to the milestone for the next release |
9a6486e to
6047090
Compare
|
rebased after #8768 |
|
CI timed out in pre-build checks in "Check source" at no clue why. It works locally. |
Mirroring 60a0ed2
Changelog-None