-
Notifications
You must be signed in to change notification settings - Fork 94
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
Update MaxEncodedLen derive macro #512
Conversation
9891cf3
to
58ac1de
Compare
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.
Could you add a test which would not compile without this change?
src/max_encoded_len.rs
Outdated
fn max_compact_encoded_len() -> usize { | ||
Self::max_encoded_len() | ||
} |
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.
Hmm... personally I'd probably prefer to keep this as a #[no_doc]
and make it an internal implementation detail, instead of making it public.
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.
yes good point
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.
by internal you mean simply adding the #[doc(hidden)]
attribute or is there other things you can do to truly keep it internal?
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.
by internal you mean simply adding the
#[doc(hidden)]
attribute or is there other things you can do to truly keep it internal?
Just hide it. I guess you could also prefix its name, e.g. prepend scale_internal_
to the name (which I did when I was adding scale_internal_decode_bytes
on Input
)
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.
I don't get why we need this at all.
The first thing should also be that there is a test added that fails with the old derive macro.
Also, a side note only tangentially related to this PR - when reviewing this code I've noticed that a compact But apparently we kinda screwed this up, and that's how we encode
Kinda unfortunate that we waste space here. |
src/max_encoded_len.rs
Outdated
impl_primitives!( | ||
i8, i16, i32, i64, i128, bool, |
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.
You know that you skipped all unsigned types?
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.
For the unsigned types the impl_compact
macro is used now, so they're not skipped. (:
src/max_encoded_len.rs
Outdated
fn max_compact_encoded_len() -> usize { | ||
Self::max_encoded_len() | ||
} |
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.
I don't get why we need this at all.
The first thing should also be that there is a test added that fails with the old derive macro.
58ac1de
to
49d2bab
Compare
); | ||
let max_len = CompactField::<u64>::max_encoded_len(); | ||
assert_eq!(max_len, Compact::<u64>::max_encoded_len() + u64::max_encoded_len()); | ||
assert_eq!(max_len, 17); | ||
} |
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.
I think I'd prefer to have a separate test for this, instead of modifying an existing one.
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.
Yes
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.
done, this test would fail with
Compiling parity-scale-codec v3.6.6 (/Users/pg/github/parity-scale-codec)
error[E0277]: the trait bound `Compact<T>: MaxEncodedLen` is not satisfied
--> tests/max_encoded_len.rs:87:5
|
87 | t: T,
| ^ the trait `MaxEncodedLen` is not implemented for `Compact<T>`
|
help: consider extending the `where` clause, but there might be an alternative better way to express this requirement
|
84 | #[derive(Encode, MaxEncodedLen, Compact<T>: MaxEncodedLen)]
| +++++++++++++++++++++++++++
```rust impl<T> ::parity_scale_codec::MaxEncodedLen for Generic<T> where T: ::parity_scale_codec::MaxEncodedLen, ``` instead of ``` impl<T> ::parity_scale_codec::MaxEncodedLen for Generic<T> where T: ::parity_scale_codec::MaxEncodedLen, T: ::parity_scale_codec::MaxEncodedLen, T: ::parity_scale_codec::MaxEncodedLen, ```
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.
Wait, why is this adding a new function max_compact_encoded_len
instead of fixing the impl of the max_encoded_len
for the Compact
wrapper?
the impl that we fixed previously #508 is not wrong, but it forces us to add extra constraint on the type when used with generics. See #512 (comment) If you have a better solution, I'll take it. |
Pushed the proper solution. We add a new required trait for We need to seal this trait. |
lgtm too, checking if everything compile properly on |
#508 fixed the generated code for MaxEncoded derive trait
This is all fine but cause some compilation errors when this field is generic, since Rust can not tell that
Compact<T>
implementsMaxEncodedLen
whenT
implements it, without extra constraints