Re: [Emacs-diffs] trunk r117340: * lisp/calculator.el: Lots of revisions

[Eli Barzilay]

On Wednesday, Stefan Monnier wrote:
> > -          (cl-letf (((symbol-function 'F)
> > -                     (lambda (&optional x y) (calculator-funcall f x y)))
> > -                    ((symbol-function 'D)
> > -                     (lambda (x) (if calculator-deg (/ (* x 180) float-pi) 
> > x))))
> > -            (eval f `((X . ,X)
> > -                      (Y . ,Y)
> > -                      (TX . ,TX)
> > -                      (TY . ,TY)
> > -                      (DX . ,DX)
> > -                      (L . ,L))))))
> > -    (error 0)))
> > +      (cl-flet ((F (&optional x y) (calculator-funcall f x y))
> > +                (D (x) (if calculator-deg (/ (* x 180) float-pi) x)))
> > +        (eval `(let ((X ,X) (Y ,Y) (DX ,DX) (TX ,TX) (TY ,TY) (L ',L))
> > +                 ,f)
> > +              t)))))
> 
> Hmm... have you tested this?  `cl-flet' creates lexically-scoped
> function bindings (contrary to the old `flet'), so the F and D above
> in your new code aren't accessible to the `f' expression.

[I know I did, since one of my commits fixed the `fib' example in the
`calculator-user-operators' docstring.  But I probably tested it with
24.3.1 which is my regular emacs, though something else must have
happened since it's broken there too now.]

Is there some way to make it work with 24.3.1 and the trunk version?
Using the original seems to complain about unbound functions when letf
is trying to save the old `symbol-function' values.


> As for changing the `eval' call, the form I used is more efficient
> than the (eval `(let ...)) you're using now: what was the motivation
> for this change?

The change I did makes it work in 24.3.1 (my regular version) which
doesn't have the new eval feature.  Efficiency is really not important
in this case, but making me test it more is important...


> >    (let ((inp (or keys (this-command-keys))))
> [...]
> > +      ;; translates kp-x to x and [tries to] create a string to lookup
> > +      ;; operators; assume all symbols are translatable via
> > +      ;; `function-key-map' or with an 'ascii-character property
> > +      (concat (mapcar (lambda (k)
> > +                        (if (numberp k) k (or (get k 'ascii-character)
> > +                                              (error "??bad key??"))))
> > +                      (or (lookup-key function-key-map inp) inp))))))
> 
> (this-command-keys) should return "fully decoded events", i.e. after
> passing through the keyboard-coding-system, input-decode-map,
> function-key-map, and key-translation-map.  So neither (lookup-key
> function-key-map inp) nor (get k 'ascii-character) should be
> necessary.
> 
> IOW if this code really is needed, it deserves a comment explaining
> why it's needed despite the fact that function-key-map was already
> applied.

It's code that has been there before, and I was never sure about it.
Here's what leads to it:

* If there is a binding for kp-whatever, rebinding whatever doesn't
  affect kp-whatever.

* Since I have (or had) a few of these, I made the code bind *both*
  kp-whatever and whatever.  (Thinking that for anyone, a good reason
  to have kp-whatever bound globally doesn't contradict calculator
  re-grabbing it.)

* With both bound, the lookup is needed, otherwise it ends up with
  [kp-7] and [kp-add] instead of "7" and "+".

The bottom line is, I'm not sure about the whole thing -- one of these
should be done:

1. The above is what it is, and needs to be synthesized into the
   comment you mention.  (Something that says that it's needed because
   kp-whatever key bindings are made.)

2. Maybe the kp-whatever key bindings should go away instead?  (I
   think that I don't need them anymore, but I hesitate to do such a
   big change in an area I don't know much about.)

3. It would really be best if there's a good way to just eliminate the
   need for the whole thing -- currently things like [kp-]7 and [kp-]+
   are bound to `calculator-op' and `calculator-digit', which need to
   lookup the actual key.  But I suppose that this will take time.

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!