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

copy-instance kwargs to overwrite slots #827

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gefjon
Copy link

@gefjon gefjon commented Jun 8, 2022

You can now do e.g. (copy-instance SOME-BINARY-CLASSICAL-INSTRUCTION :left NEW-LEFT-OPERAND) to make a copy of SOME-BINARY-CLASSICAL-INSTRUCTION that has its
left slot overwritten with NEW-LEFT-OPERAND. I suspect there are plenty of places in
the QuilC code where this would enable a much nicer functional style by eliminating
patterns like (let ((copy (copy-instance FOO))) (setf (FOO-BAR FOO) BAR)).

You can now do e.g. `(copy-instance SOME-BINARY-CLASSICAL-INSTRUCTION :left
NEW-LEFT-OPERAND)` to make a copy of `SOME-BINARY-CLASSICAL-INSTRUCTION` that has its
`left` slot overwritten with `NEW-LEFT-OPERAND`. I suspect there are plenty of places in
the QuilC code where this would enable a much nicer functional style by eliminating
patterns like `(let ((copy (copy-instance FOO))) (setf (FOO-BAR FOO) BAR))`.
@ecpeterson
Copy link
Contributor

Neat! I tried writing something like this to chase this same pattern, and I was unhappy about having to treat struct and class instances separately, so it never made it in. Some years older and wiser, I think it’s a good enough idea that it should go in anyway.

@gefjon
Copy link
Author

gefjon commented Jun 8, 2022

I think the only way to do this for subclasses of structure-object is to define individual methods, though probably via a macro. But neither the CL spec nor the MOP defines any way to write a generic method.

Oh, speaking of which, the default method for copy-instance should really specialize on standard-object, not on t. I'll make that change tomorrow and see if it breaks anything.

ETA: let's all take this as a lesson to prefer defclass over defstruct unless profiling suggests a performance incentive otherwise.

The "default" method for `copy-instance` is now specialized on `standard-object` rather
than `t`. Aside from opening the possibility that it might have been applied to instances
of `built-in-class`, the default method's applying to `structure-object`s was
non-conformant and SBCL specific (possibly other implementations as well, but certainly
not universal), as the MOP as specified in AMOP applies only to `defclass`-defined
classes, not to `defstruct` classes.

In lieu of a generic implementation for structs, we now use a macro
`define-copy-struct-instance` to define methods specific to each `structure-object`
subclass.

Pros:
- More correct; no longer relying on SBCL-specific behavior
- Potential modest performance increases, as copying structure objects no longer uses the
  MOP to dynamically compute a set of slots to copy.

Cons:
- Ugly, complex macro-ology in `define-copy-struct-instance`
- Having to define instances explicitly for structs is frustrating
- Potential bugs when a `defstruct` is incompatibly redefined without also editing its
  corresponding `define-copy-struct-object` form.
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.

2 participants