Restrict unsafe blocks to single statements where possible.#15806
Restrict unsafe blocks to single statements where possible.#15806Cryoris wants to merge 6 commits intoQiskit:mainfrom
unsafe blocks to single statements where possible.#15806Conversation
To see which part of the code is unsafe, it is good to restrict `unsafe` blocks to the smallest sensible code unit. Clippy also has a feature to check this, `cargo clippy -- -W clippy::multiple_unsafe_ops_per_block`. This commit reduces some pretty large unsafe blocks to smaller ones to improve our code structure. Clippy actually flags _any_ snippet that has more than a single statement, but I left unsafe blocks in place where every single statement was actually unsafe.
Cryoris
added
type: qa
|
One or more of the following people are relevant to this code:
|
gadial
left a comment
gadial
left a comment
There was a problem hiding this comment.
Overall looks good to me. Left minor comments.
| @@ -1569,26 +1568,22 @@ pub unsafe extern "C" fn qk_target_op_clear(op: *mut CTargetOp) { | |||
|
|
|||
| // SAFETY: As per documentation, data from pointers contained in CTargetOp | |||
| // originates from rust code and are constructed internally with vecs and CStrings. | |||
There was a problem hiding this comment.
Does this cover the mut_ptr_as_ref(op) operation?
There was a problem hiding this comment.
It does not! Let me adjust 🙂
| }; | ||
| new_node.index() as u32 | ||
| } | ||
| // SAFETY: Per the documentation the qubits pointer is an arrays of num_qubits() elements |
There was a problem hiding this comment.
| // SAFETY: Per the documentation the qubits pointer is an arrays of num_qubits() elements | |
| // SAFETY: Per the documentation the qubits pointer is an array of num_qubits() elements |
Reduce unsafe scopes further.
… into unsafe-surgeon-mode
Coverage Report for CI Build 25101306270Coverage increased (+0.02%) to 87.509%Details
Uncovered Changes
Coverage Regressions20 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
| let _ = unsafe { CString::from_raw(inst.name) }; | ||
| inst.name = std::ptr::null_mut(); | ||
| } | ||
| inst.name = std::ptr::null_mut(); |
There was a problem hiding this comment.
Isn't this causing a memory leak now because we're no longer dropping the existing string pointer? Don't we need rust to take ownership of the string again so it frees the allocation?
There was a problem hiding this comment.
Oh this looks like a merge gone wrong, I didn't mean to remove that... thanks for catching that!
| let target = if target.is_null() { | ||
| None | ||
| } else { | ||
| Some(unsafe { const_ptr_as_ref(target) }) |
There was a problem hiding this comment.
On the other places you moved the safety comment inside the else body to be with the reduced scope unsafe calls.
| pub unsafe extern "C" fn qk_neighbors_is_all_to_all(neighbors: *const CNeighbors) -> bool { | ||
| // SAFETY: per documentation, `neighbors` points to a valid initialized `CNeighbors`. | ||
| unsafe { (*neighbors).neighbors.is_null() && (*neighbors).partition.is_null() } | ||
| let neighbors = unsafe { const_ptr_as_ref(neighbors) }; |
There was a problem hiding this comment.
Doesn't this change the requirements for neighbors? This function asserts alignment while it is not documented as being required for this function.
There was a problem hiding this comment.
Hmm.. isn't that an oversight from before, though? I thought that the pointer needed to be aligned to be dereferenced safely.
6 participants
Summary
Following up on #15106 (comment) by reducing the sizes of
unsafestatements in the codebase.Details and comments
To see which part of the code is unsafe, it is good to restrict
unsafeblocks to the smallest sensible code unit. Clippy also has a feature to check this,cargo clippy -- -W clippy::multiple_unsafe_ops_per_block. This commit reduces some pretty large unsafe blocks to smaller ones to improve our code structure. Clippy actually flags any snippet that has more than a single statement, but I left unsafe blocks in place where every single statement was actually unsafe.