Conversation
| recurse_indices.push(quote_spanned! { ident.span() => | ||
| ( | ||
| (#index) as ::core::primitive::u8, | ||
| (#index) as ::core::primitive::usize, |
There was a problem hiding this comment.
I think u8 is more accurate, in the implementation of decode and encode, the type is cast to u8, if user writes:
enum Foo {
A = 256,
B = 0,
}then we want to give an error for conflicts.
The code in scale codec could be refactored to put the cast as part of the expression for variant indices but I think we are fine.
There was a problem hiding this comment.
This doesn't take the discriminant into account no?
There was a problem hiding this comment.
That is a good point. I will add a check there, still think this is better than having it fail somewhere with a weird error.
There was a problem hiding this comment.
parity-scale-codec/derive/src/utils.rs
Line 493 in 49995c8
| const indices: [(u8, &'static str); #len] = [#( #recurse_indices ,)*]; | ||
| const indices: [(usize, &'static str); #len] = [#( #recurse_indices ,)*]; | ||
|
|
||
| const fn search_for_invalid_index(array: &[(usize, &'static str); #len]) -> (bool, usize) { |
There was a problem hiding this comment.
This way we are future proofs for const variant indices.
| recurse_indices.push(quote_spanned! { ident.span() => | ||
| ( | ||
| (#index) as ::core::primitive::u8, | ||
| (#index) as ::core::primitive::usize, |
There was a problem hiding this comment.
in some very rare case it could still trigger truncation if type is like repr (u128) and he is using u128 discriminant, doesn't harm to also have the flag here: cast_possible_truncation
| }) if path.get_ident().map_or(false, |i| i == "index") => lit_int | ||
| .base10_parse::<u8>() | ||
| .map(|_| ()) | ||
| .map_err(|_| syn::Error::new(lit_int.span(), "Index must be in 0..255")), |
There was a problem hiding this comment.
In theory this check is still necessary, when we call variant_index:
parity-scale-codec/derive/src/utils.rs
Lines 138 to 140 in dacc95e
somebody writing an index more than 2^64 will make the macro panic.
Though quite unlikely.
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Closes: #713