Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.28k stars 2.37k forks source link

Move `Option<Box<_>>` into `ExtraInstructionAttributes`. #13127

Closed kevinhartman closed 2 months ago

kevinhartman commented 2 months ago

Summary

The primary motivation here is to encapsulate the hairy logic of managing memory for extra attributes. By moving the Option<Box<_>> into the ExtraInstructionAttributes struct itself, clients are no longer burdened with the responsibility of checking if the Option contains a value, unwrapping it etc.; the semantics of that aren't relevant to clients like DAGCircuit and CircuitData. Instead, the ExtraInstructionAttributes acts as a container for optional attributes, and can easily drop the Box internally if all attributes are cleared.

A give-away that this pattern could be helpful was ExtraInstructionAttributes::new's return type, which was previously Option<Self>.

Details and comments

The size of ExtraInstructionAttributes (now held by value by instructions) should be the same size as the old Option<Box<ExtraInstructionAttributes>>, so this should not have memory implications regarding the size of instructions.

Note that ExtraAttrs is completely private to the circuit_instruction module, i.e. it's only an implementation detail and in no way part of the interface.

qiskit-bot commented 2 months ago

One or more of the following people are relevant to this code:

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 10837025267

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/packed_instruction.rs 2 3 66.67%
crates/circuit/src/dag_node.rs 5 7 71.43%
crates/circuit/src/circuit_instruction.rs 54 108 50.0%
<!-- Total: 107 164 65.24% -->
Files with Coverage Reduction New Missed Lines %
crates/circuit/src/dag_circuit.rs 1 88.54%
crates/qasm2/src/expr.rs 1 94.02%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 2 88.26%
crates/qasm2/src/lex.rs 4 92.23%
crates/qasm2/src/parse.rs 6 97.61%
qiskit/synthesis/two_qubit/xx_decompose/decomposer.py 7 90.91%
<!-- Total: 21 -->
Totals Coverage Status
Change from base Build 10834414649: -0.02%
Covered Lines: 73365
Relevant Lines: 82535

💛 - Coveralls