-
Notifications
You must be signed in to change notification settings - Fork 7
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
Change compute_signature to return FunctionType (et al) #438
Conversation
@@ -227,7 +227,7 @@ impl OpDef { | |||
// TODO bring this assert back once resource inference is done? | |||
// https://github.com/CQCL-DEV/hugr/issues/425 | |||
// assert!(res.contains(self.extension())); | |||
Ok(FunctionType::new(ins, outs).with_extension_delta(&res)) | |||
Ok(res) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left this as this slightly-long-way around because of the preceding comment, which I've not tried to address in this PR. (Perhaps I could, but it's orthogonal, and looks like it might be opening another can of worms - it relates to #389 so I've noted it on there.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but alternatively one could use FunctionType::new(..., ...).with_extension_delta(...)
I think that is much preferable - makes the extension definitions more readable.
Heh, I came to the same (or nearly) conclusion independently, and have switched to There does still appear to be some confusion/disagreement in the code as to whether the ExtensionSet should include the owning Resource, but let's leave that for #389 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
....as this is now equal to the current tuple (TypeRow,TypeRow,Extensions). A longstanding TODO.
Note this does not address the confusion/disagreement across the codebase as to whether the ExtensionSet should include the resource declaring the op.