Re: [Emacs-diffs] master 1691a51: * lisp/emacs-lisp/map.el: Make the functions generic

Re: [Emacs-diffs] master 1691a51: * lisp/emacs-lisp/map.el: Make the functions generic

[Basil L. Contovounesios]

monnier@iro.umontreal.ca (Stefan Monnier) writes:

> -(defun map-empty-p (map)
> +(cl-defgeneric map-empty-p (map)
>    "Return non-nil if MAP is empty.
> +The default implementation delegates to `map-length'."
> +  (zerop (map-length map)))
> -MAP can be a list, hash-table or array."
> -  (map--dispatch map
> -    :list (null map)
> -    :array (seq-empty-p map)
> -    :hash-table (zerop (hash-table-count map))))

Why not continue to check whether lists are null, rather than traversing
their entire length:

  (cl-defmethod map-empty-p ((map list))
    (null map))

Is it not worth it?

> +(cl-defmethod map-contains-key ((map list) key &optional testfn)
> +  (alist-get key map nil nil (or testfn #'equal)))

I think this should get the same treatment as the hash table method,
which checks whether DEFAULT was returned:

  (cl-defmethod map-contains-key ((map list) key &optional testfn)
    (let ((v '(nil)))
      (not (eq v (alist-get key map v nil (or testfn #'equal))))))

Thanks,

-- 
Basil

Re: [Emacs-diffs] master 1691a51: * lisp/emacs-lisp/map.el: Make the functions generic

[Stefan Monnier]

> Why not continue to check whether lists are null, rather than traversing
> their entire length:
>
>   (cl-defmethod map-empty-p ((map list))
>     (null map))

Indeed, thanks.  Could you install this change?

>> +(cl-defmethod map-contains-key ((map list) key &optional testfn)
>> +  (alist-get key map nil nil (or testfn #'equal)))
> I think this should get the same treatment as the hash table method,
> which checks whether DEFAULT was returned:

Same here (actually, this one was a thinko: for some reason I wrote the
code assuming alist-get would behave like assoc and return the cons
cell).


        Stefan

Re: [Emacs-diffs] master 1691a51: * lisp/emacs-lisp/map.el: Make the functions generic

[Basil L. Contovounesios]

[-- Attachment #1: 0001-lisp-emacs-lisp-map.el-Fix-recent-changes.patch --]
[-- Type: text/x-diff, Size: 2396 bytes --]

From b6082c43742a4d971be3110b7d52c5eb1d52b215 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Fri, 14 Dec 2018 14:46:47 +0000
Subject: [PATCH] * lisp/emacs-lisp/map.el: Fix recent changes

(map-empty-p): Add method for lists which avoids computing their
entire length.
(map-contains-key): Check for alist membership by comparing against
DEFAULT argument returned by alist-get.
(map-put!): Reconcile argument name with that used in docstring.
---
 lisp/emacs-lisp/map.el | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/map.el b/lisp/emacs-lisp/map.el
index 35759db627..78cedd3ab1 100644
--- a/lisp/emacs-lisp/map.el
+++ b/lisp/emacs-lisp/map.el
@@ -243,6 +243,9 @@ map-nested-elt
 The default implementation delegates to `map-length'."
   (zerop (map-length map)))
 
+(cl-defmethod map-empty-p ((map list))
+  (null map))
+
 (cl-defgeneric map-contains-key (map key &optional testfn)
   ;; FIXME: The test function to use generally depends on the map object,
   ;; so specifying `testfn' here is problematic: e.g. for hash-tables
@@ -259,7 +262,8 @@ map-nested-elt
     nil))
 
 (cl-defmethod map-contains-key ((map list) key &optional testfn)
-  (alist-get key map nil nil (or testfn #'equal)))
+  (let ((v '(nil)))
+    (not (eq v (alist-get key map v nil (or testfn #'equal))))))
 
 (cl-defmethod map-contains-key ((map array) key &optional _testfn)
   (and (integerp key)
@@ -332,16 +336,16 @@ map-merge-with
 ;; FIXME: I wish there was a way to avoid this η-redex!
 (cl-defmethod map-into (map (_type (eql list))) (map-pairs map))
 
-(cl-defgeneric map-put! (map key v)
+(cl-defgeneric map-put! (map key value)
   "Associate KEY with VALUE in MAP and return VALUE.
 If KEY is already present in MAP, replace the associated value
 with VALUE."
   (map--dispatch map
     :list (let ((p (assoc key map)))
-            (if p (setcdr p v)
+            (if p (setcdr p value)
               (error "No place to change the mapping for %S" key)))
-    :hash-table (puthash key v map)
-    :array (aset map key v)))
+    :hash-table (puthash key value map)
+    :array (aset map key value)))
 
 ;; There shouldn't be old source code referring to `map--put', yet we do
 ;; need to keep it for backward compatibility with .elc files where the
-- 
2.19.2


[-- Attachment #2: Type: text/plain, Size: 393 bytes --]


Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Could you install this change?

Sorry, I don't have commit rights, so I attach the corresponding patch,
which also fixes an argument naming discrepancy in 'map-put!'.

I noticed that test/lisp/emacs-lisp/map-tests.el still uses map-put,
which is now obsolete.  Should these tests be updated to test 'map-put!'
instead?

Thanks,

-- 
Basil

Re: [Emacs-diffs] master 1691a51: * lisp/emacs-lisp/map.el: Make the functions generic

[Stefan Monnier]

> Sorry, I don't have commit rights, so I attach the corresponding patch,
> which also fixes an argument naming discrepancy in 'map-put!'.

Thanks, pushed.

> I noticed that test/lisp/emacs-lisp/map-tests.el still uses map-put,
> which is now obsolete.  Should these tests be updated to test 'map-put!'
> instead?

We should add tests for map-put!, yes.
We can keep the old tests for map-put for now, since we still
(temporarily) care about providing correct behavior for map-put.


        Stefan