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

Sam Steingold wrote:
> Sorry about not replying earlier - I thought that you were actually
> planning to work more on your patch.

Very good point. My initial reaction to Kaz' patch in
<https://sourceforge.net/p/clisp/mailman/message/36601264/>
was "who needs the generic case, the frequent case is that the keywords
are known at compile time". The only thing I did not like about it was
the (not (constantp ev-argform)) - the error checking of the keywords
should be done by DEFSETF, not in GET-SETF-EXPANSION.

But you are absolutely right: When writing compilers (and macroexpansion
code), one should strive for maximum generality, and the optimizations
of common cases are only special cases.

So, looking at
http://www.ai.mit.edu/projects/iiip/doc/CommonLISP/HyperSpec/Body/sec_5-1-1-2.html
http://www.ai.mit.edu/projects/iiip/doc/CommonLISP/HyperSpec/Body/fun_get-setf-expansion.html
http://www.ai.mit.edu/projects/iiip/doc/CommonLISP/HyperSpec/Body/mac_defsetf.html
there is even an example regarding computed keywords:

(defsetf xy (&key ((x x) 0) ((y y) 0)) (store)
  `(set-xy ,store 'x ,x 'y ,y)) =>  XY
(get-setf-expansion '(xy a b))
=> (#:t0 #:t1),
   (a b),
   (#:store),
   ((lambda (&key ((x #:x)) ((y #:y))) 
      (set-xy #:store 'x #:x 'y #:y))
    #:t0 #:t1),
   (xy #:t0 #:t1)

This example gives an important clue: How to use a LAMBDA for keyword matching.
But this example is wrong. It ignores the keyword init forms. The correct
result is:

(get-setf-expansion '(xy a b))
=> (#:t0 #:t1),
   (a b),
   (#:store),
   ((lambda (&key ((x #:x)) ((y #:y))) 
      (set-xy #:store 'x #:x 'y #:y))
    #:t0 #:t1 'x 0 'y 0),
   (xy #:t0 #:t1 'x 0 'y 0)
or
=> (#:t0 #:t1),
   (a b),
   (#:store),
   ((lambda (&key ((x #:x) 0) ((y #:y) 0)) 
      (set-xy #:store 'x #:x 'y #:y))
    #:t0 #:t1),
   (xy #:t0 #:t1 'x 0 'y 0)

Things get more hairy when we think of keyword init forms that are
not constant forms. In which environment do they get evaluated?

Example:
(defun zero () 0)
(flet ((default-form-for-y () '(zero)))
  (defsetf xy (&key ((x x) '(zero)) ((y y) (default-form-for-y))) (store)
    `(set-xy ,store 'x ,x 'y ,y))) =>  XY
There are two evaluations:
  1. At macro expansion time, i.e. during get-setf-expansion,
     (default-form-for-y) gets evaluated, in the lexical environment of the DEFSETF form.
     => (ZERO).
  2. At runtime, (ZERO) gets evaluated. => 0.

So, DEFSETF needs to store a closure that will evaluate
(default-form-for-y).

The other thing is: How should the desired result of GET-SETF-EXPANSION
look like? Let's take an example with keywords arguments that are not in
the keyword package, non-constant keyword init forms, and with a required
argument as well.

> (defsetf xy (r &key ((x x) '(zero)) ((y y) '(zero))) (store) `(set-xy ,store ,r 'x ,x 'y ,y))
XY

The simple case, when all keywords are specified as constants, is:

> (get-setf-expansion '(xy 3 'x 0 'y 0))
(#:G1 #:G2 #:G3) ;
(3 0 0) ;
(#:G4) ;
(SET-XY #:G4 #:G1 'X #:G2 'Y #:G3) ;
(XY #:G1 'X #:G2 'Y #:G3)

The general case, when all keyword arguments are non-constant, requires
the LAMBDA trick, and the mechanism for using the keyword init form
shown above:

> (get-setf-expansion '(xy 3 a 0))
(#:G1 #:G2 #:G3 #:G4 #:G5) ;
(3 A 0 (ZERO) (ZERO)) ;
(#:G6) ;
((LAMBDA (&KEY ((X #:G7)) ((Y #:G8))) (SET-XY #:G6 #:G1 'X #:G7 'Y #:G8)) #:G2 #:G3 'X #:G4 'Y #:G5) ;
(XY #:G1 #:G2 #:G3 'X #:G4 'Y #:G5)

In the mixed case, when some of the keyword arguments are constant,
the corresponding expansion can be optimized to omit the init forms
that are not needed:

> (get-setf-expansion '(xy 3 a 0 'y 0))
(#:G1 #:G2 #:G3 #:G4 #:G5) ;
(3 A 0 0 (ZERO)) ;
(#:G6) ;
((LAMBDA (&KEY ((X #:G7)) ((Y #:G8))) (SET-XY #:G6 #:G1 'X #:G7 'Y #:G8)) #:G2 #:G3 'Y #:G4 'X #:G5) ;
(XY #:G1 #:G2 #:G3 'Y #:G4 'X #:G5)

This was the hard part. Actually implementing it was easy, then.
Done and pushed (including your test case).

Bruno



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