Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changed some of the macro code so that
works, currently only inputs of owned types works.
Closes #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.