Closed jakelishman closed 1 month ago
This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Files with Coverage Reduction | New Missed Lines | % | ||
---|---|---|---|---|
crates/accelerate/src/two_qubit_decompose.rs | 1 | 90.82% | ||
crates/qasm2/src/lex.rs | 3 | 91.48% | ||
crates/qasm2/src/parse.rs | 18 | 96.69% | ||
crates/circuit/src/dag_circuit.rs | 241 | 89.02% | ||
<!-- | Total: | 263 | --> |
Totals | |
---|---|
Change from base Build 10579458520: | -0.04% |
Covered Lines: | 71633 |
Relevant Lines: | 80328 |
One or more of the following people are relevant to this code:
@Qiskit/terra-core
@kevinhartman
@mtreinish
Another API alternative here is to add an implementation of Default
/ explicit method to Interned<T> where T: ?Sized + ToOwned, <T as ToOwned>::Owned: Default
that constructs the same struct. For now I just put the method on the interner for the symmetry with get
, and because I wasn't entirely sure how the type inference would play out if it was on Interned
(I didn't want the caller to have to turbo-fish the default constructor). Feels like it could easily go either way (or both ways).
Another API alternative here is to add an implementation of Default / explicit method to Interned
where T: ?Sized + ToOwned, ::Owned: Default that constructs the same struct. For now I just put the method on the interner for the symmetry with get, and because I wasn't entirely sure how the type inference would play out if it was on Interned (I didn't want the caller to have to turbo-fish the default constructor). Feels like it could easily go either way (or both ways).
I see what you mean. I think having both could be nice, but the explicit method seems like it'd be easier for folks to find, which may result in us seeing less of .insert(&[])
in new PRs.
Added the symmetric method in 7cc72b1a5e.
What was the reason not to impl Default
itself for Interned<T>
with constraints on T
?
Otherwise, looks good.
I didn't think super hard about it, I think my default API position is just to define concrete methods rather than hide new functionality in trait implementations.
Thinking more now, I'd say that "the interned key of the default value" is not necessarily the same thing as the "default interned key", but that said: maybe my position on the trait is skewed, because imo integer types shouldn't have a default, and neither should an interner key. Judging by the API choices in both Python and Rust, though, I'm in the minority there (e.g. Python's int()
and Rust's impls of Default
for integers).
I didn't think super hard about it, I think my default API position is just to define concrete methods rather than hide new functionality in trait implementations.
I think in this case, I would actually rather we just have the get_default
like you had it before. If you want to also implement Default
, I'd be on board.
Thinking more now, I'd say that "the interned key of the default value" is not necessarily the same thing as the "default interned key"
Hmm. It's late in the night so I'm probably not picking up on the subtly, but wouldn't that break the premise of us using Default
as the trait bound for this whole thing anyhow? I guess I can see what you mean, but I'd think it'd follow that the Interner
should decide what its keys are, so in that case we should probably not let Interned
structs be constructed on their own.
I'm fine to remove it too, just let me know. If you want me to turn it into an actual Default
impl that's fine too, since the explicit method will still exist (just be called default
).
I'd say that Interned
and Interner
are so intrinsically linked that it makes little difference who decides what here - the two structs absolutely have to know how each other work in order not to violate each other's contracts. The "default" key being index 0 is fairly core to how the system works, and any risk of Interned
and Interner
getting out of sync on that numbering is the same as it was before between the Interner::new
and Interner::get_default
methods. I don't think there's a new risk by having the Interned
method because I forced the creation routines to be consistent by having get_default
call it - the risk of divergence with the Interner
constructor remains the same (and personally I'm not worried about it).
I'm not worried either 🙂. But tl;dr, I think you should just remove Interned::of_default
for now.
I don't think there's a new risk by having the Interned method because I forced the creation routines to be consistent by having get_default call it
What I meant was more that we're locking ourselves out of creating default Interned
keys statefully (in the context of an Interner
) by having a method to do it directly without an Interner
. Whereas your get_default
method on Interner
takes a &self
, so the default it produces could make use of some internal state. Does that matter? Probably not, but I don't see a reason to have both Interner::get_default
and Interned::of_default
(it doesn't feel more discoverable from an API perspective), and get_default
seems safer and more in line with the idea that we're asking an Interner
for a local reference to something within its jurisdiction.
Ok, it's gone again in b96698b381.
(Fwiw, given none of this Rust stuff is in any way public, I'm not really worried about locking ourselves out of anything - this PR's parent just changed the entire Interner
interface, for example. Keeping the constructor and the index inside the Interned
type private from ourselves stops us from leaking the "the default index is 0" abstraction anywhere - and if people were making assumptions about the indices, we'd have rather larger problems anyway.)
Summary
This makes it possible to do
Interner::get_default()
without any value, in order to retrieve a key pointing to the default allocation without making any hash lookups. While the hashing and equality check of the default allocation is typically very cheap (like the empty slice), acquiring it still generally required a function call, which often needed to be paid frequently.Details and comments
Built on top of #13033, so depends on it.