I've now implemented this but since then I've got some other thought. Which could affect whether we want this in the long run?
Cons to having this feature:
There's probably lots of edge cases it doesn't support at the token parsing level (but at least most simple ref types are supported)
It adds a bit more complexity to macro code.
Why the feature is / might not be needed.
We're passing a reference with the expectation that it will be cloned within the body of the function. Instead why not just have users use functions which take owned data and omit any extra cloning in the impl body? If the input data needs keeping, then it would be cloned and passed to the function.
This would remove the need for additional logic to use InputT as the key type when the input type is &InputT, and would defer dealing with refs and cloning to the library user.
The alternative implementation we should/cloud instead provide.
Currently the implementation provided by the cache_proc macro calls clone() on the inputs to create the key.
for simple uses of the #[cached] macro (without complexity from other macro args) this clone is needless, as there is no later borrow, so the key could consume the inputs instead.
Removing the call to clone() I think only breaks the case when the with_cached_flag is used in the tests so in that case, an alternative impl could be provided, (a cleaner way of implementing the proc macro could help here)
Further refactoring elsewhere could make this simpler, i.e. if the cache_set method could take a &Key instead of a Key, then we wouldn't need any extra logic to support ref inputs, i.e. they key type could be a ref type (maybe?)
Conclusion given the short term
I think for now, given the current implementation, supporting the inputs being references is a good idea. If the user want to maintain ownership of the function's inputs, then passing a ref makes sense and it's one less clone that needs doing for each input (i.e. fn foo(bar_arg = bar.clone()) { ... bar_arg.clone(); ... } is wasteful.
Going forward, if there is every a refactor, it would be ideal to remove the need for cloning within the generated function body, this might look like something involving cache_set taking a &Key or by maybe just only supporting owned inputs which are consumed and not cloned more than they need to be. In the later case where the cache function must consume the inputs, then I think it would be best not to support ref inputs, and have the user clone() on their side where they need it.
Changed some of the macro code so that
works, currently only inputs of owned types works.
Closes https://github.com/jaemk/cached/issues/202.
I've now implemented this but since then I've got some other thought. Which could affect whether we want this in the long run?
Cons to having this feature:
Why the feature is / might not be needed.
We're passing a reference with the expectation that it will be cloned within the body of the function. Instead why not just have users use functions which take owned data and omit any extra cloning in the impl body? If the input data needs keeping, then it would be cloned and passed to the function. This would remove the need for additional logic to use
InputT
as the key type when the input type is&InputT
, and would defer dealing with refs and cloning to the library user.The alternative implementation we should/cloud instead provide.
Currently the implementation provided by the cache_proc macro calls clone() on the inputs to create the key. for simple uses of the #[cached] macro (without complexity from other macro args) this clone is needless, as there is no later borrow, so the key could consume the inputs instead.
Removing the call to clone() I think only breaks the case when the with_cached_flag is used in the tests so in that case, an alternative impl could be provided, (a cleaner way of implementing the proc macro could help here)
Further refactoring elsewhere could make this simpler, i.e. if the cache_set method could take a
&Key
instead of aKey
, then we wouldn't need any extra logic to support ref inputs, i.e. they key type could be a ref type (maybe?)Conclusion given the short term
I think for now, given the current implementation, supporting the inputs being references is a good idea. If the user want to maintain ownership of the function's inputs, then passing a ref makes sense and it's one less clone that needs doing for each input (i.e.
fn foo(bar_arg = bar.clone()) { ... bar_arg.clone(); ... }
is wasteful.Going forward, if there is every a refactor, it would be ideal to remove the need for cloning within the generated function body, this might look like something involving cache_set taking a
&Key
or by maybe just only supporting owned inputs which are consumed and not cloned more than they need to be. In the later case where the cache function must consume the inputs, then I think it would be best not to support ref inputs, and have the user clone() on their side where they need it.