-
Notifications
You must be signed in to change notification settings - Fork 531
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
High level support and testsing for span-to-fixed-sized array. #5583
Conversation
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.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)
6561150
to
cc736d1
Compare
ae9297d
to
7c83f01
Compare
why is this needed? Code quote: SpanTryIntoEmptyFixedSizedArray<T, +Drop<T>> of TryInto<Span<T>, Box<@[T; 0]>> |
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.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)
corelib/src/array.cairo
line 274 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
why is this needed?
this just makes try-into for empty sized arrays work.
why can't we use the following signature? Suggestion: extern fn tuple_from_span<T, SIZE: u32>(span: @Array<T>) -> Option<Box<[T, SIZE]>> nopanic; |
cc736d1
to
bc2faaa
Compare
7c83f01
to
ec5d295
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.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
corelib/src/array.cairo
line 262 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
why can't we use the following signature?
no - look at the above comment.
will change it to be inner type based soon.
but currently like span_from_tuple
Previously, orizi wrote…
I read the comment and didn't understand 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.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
corelib/src/array.cairo
line 262 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
I read the comment and didn't understand it.
the only type in the generics of the libfunc is T
- the following args must not be passed to the libfunc.
since i want it to be exactly the same as span_from_tuple
i don't want to limit it to only returning the fixed_size_array
in the libfunc - so we are only dependent on the tuple like struct T
, and the size is not part of the signature.
Previously, orizi wrote…
"the following args must not be passed to the libfunc." |
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.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
corelib/src/array.cairo
line 262 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
"the following args must not be passed to the libfunc."
what does that mean?
it isn't part of the expected generic args by sierra.
Previously, orizi wrote…
how does +Copy<@t> prevent propagating the |
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.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @t)
corelib/src/array.cairo
line 262 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
how does +Copy<@t> prevent propagating the
S
type to sierra
impl args and everything following are not passed to sierra.
Previously, orizi wrote…
Why did you decide on using +Copy |
Previously, ilyalesokhin-starkware wrote…
maybe use |
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.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @t)
corelib/src/array.cairo
line 262 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
maybe use
S_
?
it is already used - and will soon be removed - rather not add boilerplate for this PR.
bc2faaa
to
1d9af3e
Compare
ec5d295
to
7814bab
Compare
1d9af3e
to
550ddf4
Compare
7814bab
to
1f25cd7
Compare
what is debox? Code quote: debox |
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.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @t)
corelib/src/test/array_test.cairo
line 149 at r3 (raw file):
Previously, ilyalesokhin-starkware wrote…
what is debox?
can you document it?
Done.
1f25cd7
to
cb14a09
Compare
550ddf4
to
7416a32
Compare
Previously, orizi wrote…
Why do you need to wrap it in a snapshot? |
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.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @t)
corelib/src/test/array_test.cairo
line 149 at r3 (raw file):
Previously, ilyalesokhin-starkware wrote…
Why do you need to wrap it in a snapshot?
The input already had a snapshot, leaving the snapshot here remove the need to require the data to have Copy
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.
Reviewed 2 of 3 files at r1, 1 of 2 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @orizi)
commit-id:d131660d
7416a32
to
3f2bbd7
Compare
Stack:
This change is