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

Consider making panic op take tuples rather than type rows #1028

Closed
cqc-alec opened this issue May 13, 2024 · 3 comments
Closed

Consider making panic op take tuples rather than type rows #1028

cqc-alec opened this issue May 13, 2024 · 3 comments

Comments

@cqc-alec
Copy link
Collaborator

cqc-alec commented May 13, 2024

It might be slightly better in terms of future-proofing to have this op take tuples of inputs and outputs, instead of rows. My reasoning is that in the future, when we have row kinded type params, we should be able to define this op without needing compute_signature.

I.e. inputs : Row. outputs : Row. panic : Error, Tuple(inputs) -> Tuple(outputs)

If we define it in terms of tuples now, there's less friction if we rewrite it this way

Originally posted by @croyzor in #1024 (review)

@acl-cqc
Copy link
Contributor

acl-cqc commented May 13, 2024

When we have row-kinded type params, we'll be able to use a typescheme to generate a panic op-node with a variable number of inputs/outputs. I presume that's what we've got ATM, but we won't need a binary compute_signature anymore.

In the meantime we could drop the binary compute_signature of #1024 if we took/returned tuples, but that moves away from the ideal interface (taking/returning multiple values) in order to simplify our implementation sooner. I wouldn't worry about that (?).

Suspect either I'm confused about what panic does or your confused about what row-variables will enable, so if not clear, let's talk IRL...

@doug-q
Copy link
Collaborator

doug-q commented May 13, 2024

I'm not up with typeschemes but I think the non-tuple interface is better, even if compute_signature is required.

@doug-q
Copy link
Collaborator

doug-q commented May 20, 2024

I'm closing as we agreed we didn't want to do this. @croyzor Please do reopen if you disagree.

@doug-q doug-q closed this as not planned Won't fix, can't repro, duplicate, stale May 20, 2024
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

No branches or pull requests

3 participants