-
Notifications
You must be signed in to change notification settings - Fork 123
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 pointwise addition for arrays and tuples #343
base: master
Are you sure you want to change the base?
Support pointwise addition for arrays and tuples #343
Conversation
Fixes JelteF#342 We want to support examples like these: ```rust struct StructRecursive { a: i32, b: [i32; 2], c: [[i32; 2]; 3], d: (i32, i32), e: ((u8, [i32; 3]), i32), f: ((u8, i32), (u8, ((i32, u64, ((u8, u8), u16)), u8))), g: i32, } struct TupleRecursive((i32, u8), [(i32, u8); 10]); ``` Supporting arrays and tuples inside of enums would also be useful, but that's not in this PR.
2777c6e
to
9250753
Compare
Unfortunately, as implemented this breaks for #[derive(Add)]
pub struct GenericArrayStruct<T> {
pub a: [T; 2],
} A workaround is: #[derive(Add)]
pub struct GenericArrayStruct<T: Copy> {
pub a: [T; 2],
} But a more proper fix would be useful. |
This alternative implementation uses iterators and vectors, but does not require `Copy`.
The latest commit on this branch has an alternative that uses |
quote! { | ||
lhs #field_path.into_iter().zip(rhs #field_path.into_iter()) | ||
.map(|(lhs, rhs)| #fn_body) | ||
.collect::<Vec<_>>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest commit on this branch has an alternative that uses
Vec
and iterators, but does not requireCopy
. I'm not sure if that's better or worse?
Nah... this is not good, as is not zero-cost and forbids no_std
usages.
I wonder whether it could be solved with some additional add-hoc type machinery, which can be put into the add
module of the derive_more
crate. We could provide either our custom Addable
trait, or a #[repr(transparent)]
wrapper type implementing the Add
trait from core
, and fill it with all the necessary blanket implementations for tuples and arrays. While in macro expansion, it would be only enough to call these implementations, which will do the job recursively and automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expanding this machinery to all types, we may support also the following case:
type MyArr = [i32; 2];
struct StructRecursive {
a: i32,
b: MyArr,
}
Which won't work if we try detecting the tuple/array type via macro.
However, I do think that the coherence won't allow this, as we will quickly hit the "upstream crates may add a new impl of trait core::ops::Add
" error. Which, in turn, could be tried to overcome with autoref-based specialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... it seems that something like the following should work in general case, allowing us to consider type aliases naturally:
#[repr(transparent)]
struct Wrap<T>(T);
impl<T, const N: usize> Add for &Wrap<[T; N]> {
// custom implementation for arrays
}
impl<T> Add for &Wrap<(T,)> {
// and so on for other tuples...
}
impl<T: Add> Add for Wrap<T> {
// piggy back implemetation to `core::ops::Add`
}
// and something like this when expanding the macro:
(&&Wrap::from(self)).add(&Wrap::from(rhs));
// where `Wrap::from` is `#[repr(transparent)]` transmuting for references
This way autoref-based specialization will try first to resolve our custom implementations for tuple and arrays, and fallback to core::ops::Add
otherwise.
Moved this to the 1.1.0 milestone as this isn't a breaking change. |
Resolves #342
Synopsis
We want to support examples like these:
Supporting arrays and tuples inside of enums would also be useful, but that's not in this PR.
Solution
Overhaul some helper functions in
add_helpers
and make them recursive.Checklist