Skip to content

Conversation

ss2165
Copy link
Member

@ss2165 ss2165 commented Oct 6, 2025

Closes #2569

@ss2165 ss2165 requested a review from a team as a code owner October 6, 2025 11:30
@ss2165 ss2165 requested review from lmondada and mark-koch and removed request for a team and lmondada October 6, 2025 11:30
Copy link

codecov bot commented Oct 6, 2025

Codecov Report

❌ Patch coverage is 89.13043% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.35%. Comparing base (4379a0f) to head (af68efd).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ugr-llvm/src/extension/collections/borrow_array.rs 87.12% 0 Missing and 13 partials ⚠️
...ore/src/std_extensions/collections/borrow_array.rs 94.59% 0 Missing and 2 partials ⚠️
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     
Flag Coverage Δ
python 91.53% <ø> (ø)
rust 82.56% <89.13%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Base automatically changed from ss/push-owxrxomqozmv to feat/barray-llvm October 6, 2025 12:55
@ss2165 ss2165 force-pushed the ss/push-vnnxrlmwsvky branch from 3541254 to eedf03d Compare October 6, 2025 12:56
@ss2165 ss2165 changed the title feat(core, llvm): add is_borrowed op for BorrowArray feat(py, core, llvm): add is_borrowed op for BorrowArray Oct 6, 2025
@ss2165 ss2165 force-pushed the ss/push-vnnxrlmwsvky branch from 6556085 to 82ea2fe Compare October 6, 2025 15:07
@acl-cqc acl-cqc force-pushed the feat/barray-llvm branch 2 times, most recently from 5be9249 to d5d5999 Compare October 7, 2025 14:59
Base automatically changed from feat/barray-llvm to main October 8, 2025 13:08
@ss2165 ss2165 force-pushed the ss/push-vnnxrlmwsvky branch from 82ea2fe to ed70a19 Compare October 8, 2025 15:09
@ss2165 ss2165 requested review from acl-cqc and removed request for mark-koch October 8, 2025 15:09
Copy link
Contributor

@acl-cqc acl-cqc left a 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();
Copy link
Contributor

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

Copy link
Member Author

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

Copy link
Contributor

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...

Copy link
Member Author

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

@ss2165 ss2165 force-pushed the ss/push-vnnxrlmwsvky branch from ed70a19 to bb9384d Compare October 8, 2025 17:15
@ss2165 ss2165 requested review from aborgna-q and acl-cqc October 8, 2025 17:16
@ss2165 ss2165 force-pushed the ss/push-vnnxrlmwsvky branch from bb9384d to af68efd Compare October 9, 2025 09:54
Comment on lines +174 to +175
vec![array_ty.clone(), usize_t],
vec![crate::extension::prelude::bool_t(), array_ty],
Copy link
Collaborator

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 🤷

@ss2165 ss2165 added this pull request to the merge queue Oct 10, 2025
Merged via the queue into main with commit 1cd08ef Oct 10, 2025
26 checks passed
@ss2165 ss2165 deleted the ss/push-vnnxrlmwsvky branch October 10, 2025 15:25
@hugrbot hugrbot mentioned this pull request Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add borrow_array.is_borrowed op

3 participants