Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support barrier;, that is barrier with no qubit args #71

Merged
merged 1 commit into from
Jan 21, 2024

Conversation

jlapeyre
Copy link
Collaborator

Closes #58

@jlapeyre jlapeyre merged commit 2d8e34f into main Jan 21, 2024
7 checks passed
@jlapeyre jlapeyre deleted the barrier-with-no-args branch January 21, 2024 18:04
@jakelishman
Copy link
Member

For this kind of thing, &Option<Vec<T>> is not typically the best return value of the getter - self.qubits.as_ref() would let us return a Option<&[T]> (Option<U>: Copy where U: Copy and immutable references are Copy), which a) has one fewer layer of pointer indirection for the caller to retrieve the slice elements and b) generally pattern-matches and binds in a more expected way for a reference.

It's not super important, it just means that in most uses, the caller has to call Option::as_ref immediately after.

@jlapeyre
Copy link
Collaborator Author

I don't follow 100%. But I get the general idea. For example, this is clearly better to me:

    pub fn qubits(&self) -> Option<&Vec<TExpr>> {
        self.qubits.as_ref()
    }

But it looks like you are asking for something else?

@jakelishman
Copy link
Member

I'm asking for this:

    pub fn qubits(&self) -> Option<&[TExpr]> {
        self.qubits.as_ref()
    }

which is more proper Rust and will automatically resolve the type-hint reference.

A &Vec is almost never necessary, because all of the interesting things you can do with something that's specifically a Vec require mutability, and all the methods that are to do with iterating/viewing items/etc are inherited by Vec<T>: Deref<Target=[T]>. In memory, a &Vec is a pointer to a (pointer, len, capacity), whereas a &[T] is directly a (pointer, len) two-tuple, so there's one less pointer indirection for the caller to access the underlying memory. (Plus, almost everything in Rust wants &[T] not &Vec, so if you've got an Option<&Vec>, you have to call (&Option)::as_deref() to coerce the inner value into the slice that's needed.

@jakelishman
Copy link
Member

Downstream code can work with &Vec - it's not actually a blocker or anything, it's just not very ergonomic.

@jlapeyre
Copy link
Collaborator Author

This

    pub fn qubits(&self) -> Option<&[TExpr]> {
        self.qubits.as_ref()
    }

gives me

error[E0308]: mismatched types
   --> crates/oq3_semantics/src/asg.rs:610:9
    |
609 |     pub fn qubits(&self) -> Option<&[TExpr]> {
    |                             ---------------- expected `Option<&[TExpr]>` because of return type
610 |         self.qubits.as_ref()
    |         ^^^^^^^^^^^^^^^^^^^^ expected `Option<&[TExpr]>`, found `Option<&Vec<TExpr>>`
    |
    = note: expected enum `Option<&[TExpr]>`
               found enum `Option<&Vec<TExpr>>`

@jakelishman
Copy link
Member

jakelishman commented Jan 22, 2024

Oh sorry, should be self.qubits.as_deref() - I forgot to edit that bit

@jlapeyre
Copy link
Collaborator Author

Ah ok.

so there's one less pointer indirection

This is definitely something we want.

@jakelishman
Copy link
Member

Honestly, it's not a blocker or anything like that, and it's probably fairly unlikely to be having any real impact on performance, it's mostly just around enabling idiomatic use of the interfaces.

@jlapeyre
Copy link
Collaborator Author

The compiler was not smart enough in this case to suggest deref.

I like the idea of using idiomatic code, because it serves as a model. and it is only extra effort to learn it the first time (or few times!). Even if in a particular case it is not important for performance.

@jakelishman
Copy link
Member

That actually could be a reasonable suggestion for an error message for rustc, I think. It's an easy mistake to make, and should be an easy alternative to check for.

jlapeyre added a commit that referenced this pull request Jan 22, 2024
See comments at bottom of #71
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.

No-argument form of barrier unsupported in semantic analysis
2 participants