Skip to content

Conversation

@ExPixel
Copy link

@ExPixel ExPixel commented Sep 26, 2020

Since register_local_helper and get_local_helper already make use of Rc in their signatures I figured there wouldn't be any harm in providing a way to create helpers using Rc directly without the reallocation required by Rc::from_box. It also allows using the same helper definition for multiple helpers with different names.

I also thought about fn register_local_helper<T: Into<Rc<dyn HelperDef + 'rc>>> but I think that might mess with type inference.

@sunng87
Copy link
Owner

sunng87 commented Oct 1, 2020

Thank you for your patch! Instead of creating a new method for this, I'm +1 for your idea of using generics. I'm working on 4.0 so even a little break change is acceptable at the moment.

Also implemented `HelperDef` directly on Rc's and Arc's
so that they can be passed directly into `register_local_helper`.
With this change we can call `register_local_helper(helper)` instead
of `register_local_helper(Box::new(helper))`.
@ExPixel
Copy link
Author

ExPixel commented Oct 1, 2020

So I went ahead and just made it generic in a bit of a different way. This should still allow Rc<dyn HelperDef> (I had to implement that one manually) and Box<dyn HelperDef> but should also just allow passing closures or anything else that implements HelperDef. I can change render_helper's API and usages as well if this seems good.

@sunng87
Copy link
Owner

sunng87 commented Oct 2, 2020

This looks great to me! Please go ahead.

@ExPixel
Copy link
Author

ExPixel commented Oct 2, 2020

Alright, so upon further inspection (and after more sleep) this doesn't really do what I set out for it to. The Rc::from(T) call is just going to produce an Rc<Rc<T>>. Without specialization there isn't really a way for me to implement things in a way that will allow T where T: HelperDef, Box<T> where T: HelperDef and Rc<T> where T: HelperDef to be used without extra allocations for Box<T> in Handlebars::register_helper and for Rc<T> in RenderContext::register_local_helper. Requiring Into<Rc<dyn HelperDef>> works but it messes up type inference and makes passing Box::new(new_helper) require some inconvenient type gymnastics. Going to have to mess around with trait bounds a bit more to get something that works decently well.

@sunng87 sunng87 force-pushed the master branch 2 times, most recently from 7abfce6 to 4962d7e Compare June 4, 2021 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants