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

More 'mixed' instances to simplify Expression arithmetic #479

Merged
merged 11 commits into from
Nov 1, 2024

Conversation

matthiasgoergens
Copy link
Collaborator

@matthiasgoergens matthiasgoergens commented Oct 28, 2024

We implement convenient instances to add / multiply / subtract references to expressions.

Nothing changes under the hood, but we move some cloning from the caller to the implementation of arithmetic operations.

@matthiasgoergens
Copy link
Collaborator Author

This used to build on #450, but that's now already merged.

@yczhangsjtu
Copy link
Collaborator

I see there are still some clone() in the implementation of the operators. Is it because the original implementation is for Expression<E> instead of &Expression<E>? Is it possible to put the original implementation in impl &Expression<E>, and let other implementations refer to this, instead of the other way round?

@matthiasgoergens
Copy link
Collaborator Author

I see there are still some clone() in the implementation of the operators. Is it because the original implementation is for Expression<E> instead of &Expression<E>?

Yes, that's exactly right.

Is it possible to put the original implementation in impl &Expression<E>, and let other implementations refer to this, instead of the other way round?

Alas, not with the current design. I actually have a prototype PR to enable exactly that. But that's a much larger refactoring, and I still have so many unreviewed clean up PRs left.

@matthiasgoergens matthiasgoergens merged commit 684a762 into master Nov 1, 2024
6 checks passed
@matthiasgoergens matthiasgoergens deleted the matthias/toexpr branch November 1, 2024 05:13
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