bug#17630: 24.3.91; gv expander for a few places are incorrect

bug#17630: 24.3.91; gv expander for a few places are incorrect

[Leo Liu]

Hi Stefan,

These are incorrect:

(gv-define-simple-setter window-buffer set-window-buffer)
(gv-define-simple-setter window-display-table set-window-display-table 'fix)
(gv-define-simple-setter window-dedicated-p set-window-dedicated-p)
(gv-define-simple-setter window-hscroll set-window-hscroll)
(gv-define-simple-setter window-point set-window-point)
(gv-define-simple-setter window-start set-window-start)

The getter allows optional WINDOW arg but the setter requires WINDOW
arg. For example:

  (setf (window-buffer) (get-buffer "abc")) expands incorrectly to
  (set-window-buffer (get-buffer "abc"))

They should probably all be re-defined using gv-define-setter.

Leo

bug#17630: 24.3.91; gv expander for a few places are incorrect

[Stefan Monnier]

> The getter allows optional WINDOW arg but the setter requires WINDOW
> arg. For example:

>   (setf (window-buffer) (get-buffer "abc")) expands incorrectly to
>   (set-window-buffer (get-buffer "abc"))

> They should probably all be re-defined using gv-define-setter.

Indeed.  Tho maybe a better fix is to get rid of the asymmetry between
the getter and the setter.


        Stefan

bug#17630: 24.3.91; gv expander for a few places are incorrect

[Leo Liu]

On 2014-05-29 09:18 -0400, Stefan Monnier wrote:
> Indeed.  Tho maybe a better fix is to get rid of the asymmetry between
> the getter and the setter.

Yes but the situation is both odd and logical. Due to the naming of the
setter the WINDOW arg naturally should appear first. For example:

  (set-window-parameter PARAMETER VALUE &optional WINDOW)

is less clear. Making the WINDOW arg required in the getter has
downsides as well.

Leo

bug#17630: 24.3.91; gv expander for a few places are incorrect

[Stefan Monnier]

> Yes but the situation is both odd and logical. Due to the naming of the
> setter the WINDOW arg naturally should appear first. For example:
>   (set-window-parameter PARAMETER VALUE &optional WINDOW)
> is less clear. Making the WINDOW arg required in the getter has
> downsides as well.

Indeed.  So I guess the better fix is to use gv-define-setter, indeed.
BTW, IIRC these setter definitions come straight from the old CL code,
so this bug has been with us for a while.


        Stefan

bug#17630: 24.3.91; gv expander for a few places are incorrect

[Leo Liu]

On 2014-05-30 12:20 -0400, Stefan Monnier wrote:
> Indeed.  So I guess the better fix is to use gv-define-setter, indeed.
> BTW, IIRC these setter definitions come straight from the old CL code,
> so this bug has been with us for a while.

Indeed, though the return value of (setf (window-buffer ...) ...) is a
regression.

The fix is trivial. Do you mind installing it in emacs-24?

=== modified file 'lisp/emacs-lisp/gv.el'
--- lisp/emacs-lisp/gv.el	2014-02-21 19:01:19 +0000
+++ lisp/emacs-lisp/gv.el	2014-05-31 02:50:44 +0000
@@ -340,13 +340,16 @@
 (gv-define-simple-setter process-filter set-process-filter)
 (gv-define-simple-setter process-sentinel set-process-sentinel)
 (gv-define-simple-setter process-get process-put)
-(gv-define-simple-setter window-buffer set-window-buffer)
-(gv-define-simple-setter window-display-table set-window-display-table 'fix)
-(gv-define-simple-setter window-dedicated-p set-window-dedicated-p)
-(gv-define-simple-setter window-hscroll set-window-hscroll)
 (gv-define-simple-setter window-parameter set-window-parameter)
-(gv-define-simple-setter window-point set-window-point)
-(gv-define-simple-setter window-start set-window-start)
+(gv-define-setter window-buffer (v &optional w)
+  `(progn (set-window-buffer ,w ,v) ,v))
+(gv-define-setter window-display-table (v &optional w)
+  `(progn (set-window-display-table ,w ,v) ,v))
+(gv-define-setter window-dedicated-p (v &optional w)
+  `(set-window-dedicated-p ,w ,v))
+(gv-define-setter window-hscroll (v &optional w) `(set-window-hscroll ,w ,v))
+(gv-define-setter window-point (v &optional w) `(set-window-point ,w ,v))
+(gv-define-setter window-start (v &optional w) `(set-window-start ,w ,v))
 
 (gv-define-setter buffer-local-value (val var buf)
   (macroexp-let2 nil v val

bug#17630: 24.3.91; gv expander for a few places are incorrect

[Stefan Monnier]

> The fix is trivial. Do you mind installing it in emacs-24?

I think it's OK, go ahead.


        Stefan

bug#17630: 24.3.91; gv expander for a few places are incorrect

[Stefan Monnier]

> +(gv-define-setter window-buffer (v &optional w)
> +  `(progn (set-window-buffer ,w ,v) ,v))

Actually, this is wrong.  C-h f gv-define-setter says:

   The first arg in ARGLIST (the one that receives VAL) receives an
   expression which can do arbitrary things, whereas the other arguments
   are all guaranteed to be pure and copyable.

So using ,v twice will cause havoc in things like

   (setf (window-buffer foo) (pop buffer))


-- Stefan

bug#17630: 24.3.91; gv expander for a few places are incorrect

[Leo Liu]

Fixed in 24.4.

On 2014-05-31 10:37 -0400, Stefan Monnier wrote:
> So using ,v twice will cause havoc in things like
>
>    (setf (window-buffer foo) (pop buffer))

The curse of multiple evaluation. I think I have corrected this.

Thanks,
Leo