Skip to content
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

Support &T and Option<&T> in input #203

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BaxHugh
Copy link
Contributor

@BaxHugh BaxHugh commented Apr 9, 2024

Changed some of the macro code so that

#[cached]
fn my_func(a: &T, b: Option<&T>) -> U;

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:

  • 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.

@BaxHugh
Copy link
Contributor Author

BaxHugh commented Apr 9, 2024

Note: I think #196 seems relevant

@BaxHugh BaxHugh force-pushed the feature/support-ref-types-in-input branch from 16d7d23 to de3285c Compare April 26, 2024 09:14
- make_cache_key_type converts keys of Option<&T> to Option<T>
- Use the __private ToFullyOwned trait in generated code impl
@BaxHugh BaxHugh force-pushed the feature/support-ref-types-in-input branch from de3285c to 5b28035 Compare May 20, 2024 14:34
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.

proc_macro: support args which are &T and Option<&T>
1 participant