Accept Option<&str> instead of &Option<String>, etc

[jlapeyre]

Accept a wider range of input types, and more idiomatic input types in a few functions. This affects code added in the gates-in-rust PR.

In a few places, this removes unnecessary object copies.

DONE: The same change could be made in several other places. I can add these in another commit to this PR.

[qiskit-bot]

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

• @Eric-Arellano
• @Qiskit/terra-core
• @kevinhartman
• @mtreinish

[jakelishman]

This is a good idea in general, thanks. I have a branch locally that does much of this as part of a different performance change, but I can update that.

[jakelishman]

I think this particular one is mistaken; it's correct that the label is owned in this constructor.

[jlapeyre]

My intention was to move the creation of the String into the constructor rather than the caller. Is the problem that this method py_new is called from Python and has to build a String rather than &str? Then a ref is passed and another String is created with label.map(|x| x.to_string()) which is of course wasteful ?

[jakelishman]

What I'm thinking of is that if the label is Some, then we're always going to need to get an owned object. There's no situation in which we pass Some to the label and we won't need to clone it to get an owned copy, so there's nothing gained by allowing it to accept a ref, and there's something potentially lost if the caller had to create the label specifically (say from a format!) but can't give over the ownership.

I don't know 100% what PyO3 does when it's got an incoming Bound<'py, PyString> (i.e. if it makes a String or directly a &str - I suspect the latter), but accepting an owned String allows all possible callers to use the most efficient form.

[jakelishman]

This is similarly correct that it's actually able to transfer the ownership.

[jlapeyre]

Hmmm. I guess if we want to transfer ownership, then there is no advantage to building a String in the constructor. Instead require that the caller constructs a String. And move it. Otherwise you construct the String twice, once in the caller and once in the callee.

[jakelishman]

#12594 is my PR that'll clash, btw - I can update that, though.

[jlapeyre]

That was faster than I expected. I fixed some clippy complaints and rebased in order to force push because surely no one has looked at this PR yet. I'll push it anyway. I'll compare to your PR; maybe this one is not necessary.

[coveralls]

Pull Request Test Coverage Report for Build 9551983033

Details

• 23 of 25 (92.0%) changed or added relevant lines in 3 files are covered.
• 21 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.02%) to 89.726%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/circuit_data.rs	11	13	84.62%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	3	92.37%
crates/qasm2/src/parse.rs	18	96.69%

Totals
Change from base Build 9551377748:	-0.02%
Covered Lines:	63398
Relevant Lines:	70657

---

💛 - Coveralls

[jakelishman]

I'm fine to merge this (or some form of it) before mine - the similar changes in mine are coincidental.

[jlapeyre]

I'm fine to merge this (or some form of it) before mine - the similar changes in mine are coincidental.

Ok. Then I should 1) correct the observations you made above and 2) make the same changes where possible in the signatures.

[coveralls]

Pull Request Test Coverage Report for Build 9670588700

Details

• 4 of 4 (100.0%) changed or added relevant lines in 3 files are covered.
• 2 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.03%) to 89.752%

---

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	94.02%
crates/qasm2/src/lex.rs	1	93.38%

Totals
Change from base Build 9661630219:	0.03%
Covered Lines:	63767
Relevant Lines:	71048

---

💛 - Coveralls