-
Notifications
You must be signed in to change notification settings - Fork 14
feat(py, core, llvm): add is_borrowed
op for BorrowArray
#2610
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2610 +/- ##
==========================================
+ Coverage 83.32% 83.35% +0.02%
==========================================
Files 256 256
Lines 50491 50618 +127
Branches 46014 46141 +127
==========================================
+ Hits 42074 42195 +121
+ Misses 6065 6061 -4
- Partials 2352 2362 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3541254
to
eedf03d
Compare
is_borrowed
op for BorrowArrayis_borrowed
op for BorrowArray
91f42b2
to
439f09c
Compare
6556085
to
82ea2fe
Compare
5be9249
to
d5d5999
Compare
82ea2fe
to
ed70a19
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.
Concerns about use of word "check" is pure pedantry, but I'm not convinced about including this as a MaskCheck
DFGBuilder::new(Signature::new(vec![arr_ty.clone()], vec![qb_t(), arr_ty])) | ||
.unwrap(); | ||
let idx = builder.add_load_value(ConstUsize::new(2)); | ||
let [arr] = builder.input_wires_arr(); |
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.
do an is-borrowed before too, and return two booleans
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.
don't see the reason, this is not an execution test, just of building the op
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.
Well I was thinking you could use this in the execution test...
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 general pattern is to keep the tests very independent and I would like to maintain that
ed70a19
to
bb9384d
Compare
chore: update extension
bb9384d
to
af68efd
Compare
vec![array_ty.clone(), usize_t], | ||
vec![crate::extension::prelude::bool_t(), array_ty], |
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 type order is a bit weird, with the array in first place for the input but 2nd in the output.
But I see that we're doing the same with borrow
so 🤷
Closes #2569