-
Notifications
You must be signed in to change notification settings - Fork 105
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
feat(core): Add From<[T; N]> for LimitedVec #3647
Conversation
Just threw this little test together:
Where if the line is uncommented, compilation fails with this error:
I personally think this would be better than runtime assertion, but I do not know whether I can use this feature since it is techincally unstable, though should not arise any problems in this scenario. |
Another option is using #![feature(associated_const_equality)]
use core::marker::PhantomData;
pub struct LimitedVec<T, E, const N: usize>(Vec<T>, PhantomData<E>);
macro_rules! const_conditions {
($($name:ident = |$($arg:ident: $argtype:ty),*| $result:expr;)*) => {
$(
enum $name<$(const $arg: $argtype),*> {}
impl<$(const $arg: $argtype),*> $crate::ConstCondition for $name<$($arg),*> {
const RESULT: bool = $result;
}
)*
}
}
trait ConstCondition {
const RESULT: bool;
}
const_conditions! {
LessThan = |L: usize, R: usize| L < R;
}
impl<T, E, const M: usize, const N: usize> From<[T; M]> for LimitedVec<T, E, N>
where
LessThan<N, M>: ConstCondition<RESULT = false>,
{
fn from(value: [T; M]) -> Self {
Self(value.into(), PhantomData)
}
} Or, without any macros, #![feature(associated_const_equality)]
trait ConstCondition {
const RESULT: bool;
}
enum LessThan<const L: usize, const R: usize> {}
impl<const L: usize, const R: usize> ConstCondition for LessThan<L, R> {
const RESULT: bool = L < R;
}
impl<T, E, const M: usize, const N: usize> From<[T; M]> for LimitedVec<T, E, N>
where
LessThan<N, M>: ConstCondition<RESULT = false>,
{
fn from(value: [T; M]) -> Self {
Self(value.into(), PhantomData)
}
} |
That would probably be a better solution. |
Note that there is a drawback: you can not just |
@@ -37,6 +37,13 @@ use scale_info::{ | |||
#[derive(Clone, Default, Eq, Hash, Ord, PartialEq, PartialOrd, Decode, Encode, TypeInfo)] | |||
pub struct LimitedVec<T, E, const N: usize>(Vec<T>, PhantomData<E>); | |||
|
|||
impl<T, E, const M: usize, const N: usize> From<[T; M]> for LimitedVec<T, E, N> { |
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'm against of assertions in runtime for such a type, that's mainly used in non-test code. It's not worth adding potentially not checked to be unreachable by tests panic in the trait impl that's needed only for tests.
If there's no other stable solution, then it seems better to close the PR and wait for stable.
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.
+1
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.
There is no way to use static assertions without unstable features. static_assert!() doesnt work with const generics and everything else is gated behind unstable features. Unless it is fine to use them, which you have specified it isn't, then I'm going to close this PR until some feature becomes stable.
Try this: nvzqz/static-assertions#40 (comment) |
impl<T, E, const M: usize, const N: usize> From<[T; M]> for LimitedVec<T, E, N> { | ||
fn from(value: [T; M]) -> Self { | ||
assert!(M <= N); | ||
Self(value.into(), Default::default()) | ||
} | ||
} | ||
|
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.
impl<T, E, const M: usize, const N: usize> From<[T; M]> for LimitedVec<T, E, N> { | |
fn from(value: [T; M]) -> Self { | |
assert!(M <= N); | |
Self(value.into(), Default::default()) | |
} | |
} | |
struct LessThenOrEqual<const M: usize, const N: usize> {} | |
impl<const M: usize, const N: usize> LessThenOrEqual<M, N> { | |
const ASSERTION: () = assert!(M <= N); | |
} | |
impl<T, E, const M: usize, const N: usize> From<[T; M]> for LimitedVec<T, E, N> { | |
fn from(value: [T; M]) -> Self { | |
let _ = LessThenOrEqual::<M, N>::ASSERTION; | |
Self(value.into(), Default::default()) | |
} | |
} |
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.
please also replace places where ::from
can be used
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.
this is still runtime assertion isnt it?
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.
this is still runtime assertion isnt it?
No
static assertions unfortunately cant work on const generics |
Since it is not preferable to use runtime assertions, and all static assertions I am aware of are gated behind unstable features, which I shouldn't use. I am going to close this PR. |
Resolves #3641
where [(); {N - M}]:
, however, it needs#![feature(generic_const_expr)]
. If you think this is better I can change it to that@reviewer-or-team