Re: [PATCH] Fix broken defsetf for places with keywords.

On 2019-06-28 07:57, Sam Steingold wrote:
> Hi Kaz,
> 
> Sorry about not replying earlier - I thought that you were actually
> planning to work more on your patch.
> 
>> * Kaz Kylheku <[email protected]> [2019-03-01 16:45:18 -0800]:
>> 
>> Note the code is not prepared to deal with situations when the
>> keywords are given by evaluated expressions. I am not fixing that
>> aspect of it, and it's probably a bad idea; a virtue of the defsetf
>> logic over keyword arguments is that it understands keyword
>> arguments.
> 
> How do other implementations handle this?

Who cares. :)

> It is unclear if this aspect _can_ be easily fixed.
> Both CLISP and SBCL fail identically:
> 
> 
> ```
> (defparameter *xy* (make-array '(10 10) :initial-element NIL))
> (defun xy-s (&key ((x x) 0) ((y y) 0)) (aref *xy* x y))
> (defun xy-k (&key (x 0) (y 0)) (aref *xy* x y))
> (defun set-xy-s (new-value &key ((x x) 0) ((y y) 0))
>   (setf (aref *xy* x y) new-value))
> (defun set-xy-k (new-value &key (x 0) (y 0))
>   (setf (aref *xy* x y) new-value))
> (defsetf xy-s (&key ((x x) 0) ((y y) 0)) (store)
>   `(set-xy-s ,store 'x ,x 'y ,y))
> (defsetf xy-k (&key (x 0) (y 0)) (store)
>   `(set-xy-k ,store :x ,x :y ,y))
> 
> (assert (eql NIL (xy-k :x 1)))
> (assert (eql NIL (xy-s 'x 1)))
> (assert (eql 10 (setf (xy-k :x 1) 10)))
> (assert (eql 20 (setf (xy-s 'x 2) 20)))

Unmodified CLISP fails on the above line:

    The argument 'X to XY-S should be a keyword

Patched works. That is representative of what I'm trying to fix.

> (assert (eql 20 (xy-k :x 20)))
> (assert (eql 10 (xy-s 'x 10)))

The immediately preceding two expressions attempt an OOB array access;
I commented them out.

> (let ((a 'x) (b 'y))
>   (setf (xy-s a 1 b 2) 3)
>   (setf (xy-s b 5 a 9) 14))

Patched CLISP still fails on the *let*, as I expect. That is not 
supported,
and cannot easily be.

Note that even if it were supported, it wouldn't work for your test 
cases
here because ... your store form hard codes the symbols:

   (set-xy-s ,store 'x ,x 'y ,y)

I don't see a way of fixing this; how will the scope of your store form
receive the value?  In other words:

   (set-xy-s ,store ,what-goes-here ,x ,and-here ,y)  ?

those symbols have to come from the defsetf syntax somewhere.

If someone wants this kind of run-time behavior
with keywords, what they can do is imply register the function
with the short form of defset:

   [1]> (defsetf foo set-foo)
   FOO
   [2]> (macroexpand '(setf (foo :x 1 :y 1) 42))
   (SET-FOO :X 1 :Y 1 42) ;

Right? This approach will blindly take any arguments and gensymize
everything:

   [3]> (macroexpand '(incf (foo (h) (i) (j) (k))))
   (LET*
    ((#:TEMP-3362 (H)) (#:TEMP-3361 (I)) (#:TEMP-3360 (J)) (#:TEMP-3359 
(K))
     (#:NEW-3358 (+ (FOO #:TEMP-3362 #:TEMP-3361 #:TEMP-3360 #:TEMP-3359) 
1)))
    (SET-FOO #:TEMP-3362 #:TEMP-3361 #:TEMP-3360 #:TEMP-3359 #:NEW-3358)) 
;

the setter is agnostic about keywords; only the function knows
about them and that's where any error checking takes place.

I think that the support for keyword arguments in DEFSETF
is specifically to give the programmer an alternative from the above
loose treatment.

Anyway, the static treatment of the indicator symbols is, IIRC, deeply
designed into the strategy used by CLISP's expander. Making it somehow
work more flexibly is out of scope of my bugfix.

> (assert (eql 3 (xy-s 'y 2 'x 1)))
> (assert (eql 3 (xy-k :y 2 :x 1)))
> (assert (eql 14 (xy-k :x 9 :y 5)))
> (assert (eql 14 (xy-s 'x 9 'y 5)))

I've moved these into the LET; they die with it.

> (setf (xy-k (if t :x :y) 44 (if nil :x :y) 22))

You have a missing new value here; I made it ..y) 22) 42).

No, computed keywords don't work also for the same reason;
SETF won't evaluate those arguments. The diagnostic isn't great:

   "illegal keyword/value pair #:G2971, #:G2972 in argument list
    The allowed keywords are (:X :Y)."

But the second sentence of tells the truth, at least.

Cheers ...



_______________________________________________
clisp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/clisp-list