unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* map-put! and (setf (map-elt ...) ..) on lists
@ 2018-12-14 17:32 Stefan Monnier
  2018-12-16 16:32 ` Tom Tromey
  2018-12-17 11:38 ` Nicolas Petton
  0 siblings, 2 replies; 20+ messages in thread
From: Stefan Monnier @ 2018-12-14 17:32 UTC (permalink / raw)
  To: Nicolas Petton; +Cc: emacs-devel

The current handling of map-put on lists is very ad-hoc:
The gv-expander of `map-elt` tests if the arg is a list and if so
delegates to `alist-get`.

It kind of works, but for a library that's supposed to be generic and
expandable to other map types, this is undesirable.

So in the patch below I change this such that `map-elt` does not special
case lists any more.  Instead `map-put!` is changed to signal a special
error when it can't do its job, and the gv-expander of `map-elt` catches
this error and delegates the job to a new non-side-effecting
`map-insert`.

With this, we can add new map types via defmethod that work like lists
(i.e. that don't support inplace update but can still be modified via
`setf`).

WDYT?


        Stefan


diff --git a/lisp/emacs-lisp/map.el b/lisp/emacs-lisp/map.el
index 78cedd3ab1..d5051fcd98 100644
--- a/lisp/emacs-lisp/map.el
+++ b/lisp/emacs-lisp/map.el
@@ -95,12 +95,13 @@ map-let
            (t (error "Unsupported map type `%S': %S"
                      (type-of ,map-var) ,map-var)))))
 
+(define-error 'map-not-inplace "Cannot modify map in-place: %S")
+
 (cl-defgeneric map-elt (map key &optional default testfn)
   "Lookup KEY in MAP and return its associated value.
 If KEY is not found, return DEFAULT which defaults to nil.
 
 TESTFN is deprecated.  Its default depends on the MAP argument.
-If MAP is a list, the default is `eql' to lookup KEY.
 
 In the base definition, MAP can be an alist, hash-table, or array."
   (declare
@@ -110,15 +111,16 @@ map-let
         (macroexp-let2* nil
             ;; Eval them once and for all in the right order.
             ((key key) (default default) (testfn testfn))
-          `(if (listp ,mgetter)
-               ;; Special case the alist case, since it can't be handled by the
-               ;; map--put function.
-               ,(gv-get `(alist-get ,key (gv-synthetic-place
-                                          ,mgetter ,msetter)
-                                    ,default nil ,testfn)
-                        do)
-             ,(funcall do `(map-elt ,mgetter ,key ,default)
-                       (lambda (v) `(map-put! ,mgetter ,key ,v)))))))))
+          (funcall do `(map-elt ,mgetter ,key ,default)
+                   (lambda (v)
+                     `(condition-case nil
+                          ;; Silence warnings about the hidden 4th arg.
+                          (with-no-warnings (map-put! ,mgetter ,key ,v ,testfn))
+                        (map-not-inplace
+                         ,(funcall msetter
+                                   `(map-insert ,mgetter ,key ,v))))))))))
+   ;; `testfn' is deprecated.
+   (advertised-calling-convention (map key &optional default) "27.1"))
   (map--dispatch map
     :list (alist-get key map default nil testfn)
     :hash-table (gethash key map default)
@@ -336,17 +338,36 @@ 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 value)
+(cl-defgeneric map-put! (map key value &optional testfn)
   "Associate KEY with VALUE in MAP and return VALUE.
 If KEY is already present in MAP, replace the associated value
-with VALUE."
+with VALUE.
+This operates by modifying MAP in place.
+If it cannot do that, it signals the `map-not-inplace' error.
+If you want to insert an element without modifying MAP, use `map-insert'."
+  ;; `testfn' only exists for backward compatibility with `map-put'!
+  (declare (advertised-calling-convention (map key value) "27.1"))
   (map--dispatch map
-    :list (let ((p (assoc key map)))
-            (if p (setcdr p value)
-              (error "No place to change the mapping for %S" key)))
+    :list (let ((oldmap map))
+            (setf (alist-get key map key nil (or testfn #'equal)) value)
+            (unless (eq oldmap map)
+              (signal 'map-not-inplace (list map))))
     :hash-table (puthash key value map)
+    ;; FIXME: If `key' is too large, should we signal `map-not-inplace'
+    ;; and let `map-insert' grow the array?
     :array (aset map key value)))
 
+(define-error 'map-inplace "Can only modify map in place: %S")
+
+(cl-defgeneric map-insert (map key value)
+  "Return a new map like MAP except that it associates KEY with VALUE.
+This does not modify MAP.
+If you want to insert an element in place, use `map-put!'."
+  (if (listp map)
+      (cons (cons key value) map)
+    ;; FIXME: Should we signal an error or use copy+put! ?
+    (signal 'map-inplace (list map))))
+
 ;; 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
 ;; expansion of `setf' may call this function.
diff --git a/test/lisp/emacs-lisp/map-tests.el b/test/lisp/emacs-lisp/map-tests.el
index 885b09be98..40ebb86e80 100644
--- a/test/lisp/emacs-lisp/map-tests.el
+++ b/test/lisp/emacs-lisp/map-tests.el
@@ -76,13 +76,25 @@ with-maps-do
                          'b
                          '2))))
 
-(ert-deftest test-map-put ()
+(ert-deftest test-map-put! ()
   (with-maps-do map
     (setf (map-elt map 2) 'hello)
     (should (eq (map-elt map 2) 'hello)))
   (with-maps-do map
     (map-put map 2 'hello)
     (should (eq (map-elt map 2) 'hello)))
+  (with-maps-do map
+    (map-put! map 2 'hello)
+    (should (eq (map-elt map 2) 'hello))
+    (if (not (hash-table-p map))
+        (should-error (map-put! map 5 'value)
+                      :type (if (listp map)
+                                'map-not-inplace
+                              ;; For vectors, it could arguably signal
+                              ;; map-not-inplace as well, but it currently doesn't.
+                              'error))
+      (map-put! map 5 'value)
+      (should (eq (map-elt map 5) 'value))))
   (let ((ht (make-hash-table)))
     (setf (map-elt ht 2) 'a)
     (should (eq (map-elt ht 2)
@@ -92,7 +104,7 @@ with-maps-do
     (should (eq (map-elt alist 2)
                 'a)))
   (let ((vec [3 4 5]))
-   (should-error (setf (map-elt vec 3) 6))))
+    (should-error (setf (map-elt vec 3) 6))))
 
 (ert-deftest test-map-put-alist-new-key ()
   "Regression test for Bug#23105."
@@ -105,9 +117,9 @@ with-maps-do
   (let ((alist (list (cons "a" 1) (cons "b" 2)))
         ;; Make sure to use a non-eq "a", even when compiled.
         (noneq-key (string ?a)))
-    (map-put alist noneq-key 3 'equal)
+    (map-put alist noneq-key 3 #'equal)
     (should-not (cddr alist))
-    (map-put alist noneq-key 9)
+    (map-put alist noneq-key 9 #'eql)
     (should (cddr alist))))
 
 (ert-deftest test-map-put-return-value ()



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-14 17:32 map-put! and (setf (map-elt ...) ..) on lists Stefan Monnier
@ 2018-12-16 16:32 ` Tom Tromey
  2018-12-16 17:37   ` Drew Adams
  2018-12-17 11:38 ` Nicolas Petton
  1 sibling, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2018-12-16 16:32 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Nicolas Petton, emacs-devel

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

Stefan> -(cl-defgeneric map-put! (map key value)
Stefan> +(cl-defgeneric map-put! (map key value &optional testfn)

I wish Emacs would not adopt the Scheme "!" convention.  I find it very
distracting when reading code, as if it is shouting all the time.  Also,
IMO, it's not normally the case that modifications are so important that
they have to be called out; and anyway if they are, it would be better
to leave this to person reading the code rather than the person writing
it.

Tom



^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-16 16:32 ` Tom Tromey
@ 2018-12-16 17:37   ` Drew Adams
  2018-12-16 18:20     ` Stefan Monnier
  2018-12-16 18:21     ` Stefan Monnier
  0 siblings, 2 replies; 20+ messages in thread
From: Drew Adams @ 2018-12-16 17:37 UTC (permalink / raw)
  To: Tom Tromey, Stefan Monnier; +Cc: Nicolas Petton, emacs-devel

> I wish Emacs would not adopt the Scheme "!" convention.  I find it very distracting when reading code, as if it is shouting all the time.

> Also, IMO, it's not normally the case that modifications are so important that they have to be called out; and anyway if they are, it would be better to leave this to person reading the code rather than the person writing it.

What Tom said.  But wait, there's more...

It might make some sense for Scheme, especially in
its use for teaching, but it doesn't make sense for
Lisp (er, non-Scheme Lisp, if you prefer), where
anything pure / side-effect free is the exception
rather than the rule.

Not to mention that tossing even a single impure
grain into an otherwise pure recipe can make the
entire cake you bake unclean.

Are you going to suffix the name of each function
you define with `!', to warn its users that it
might perform side effects?  Will coders hang such
"danger" signs all over the place, the `!' effect
spreading by simple contact, like a virus?

Of course not - in practice, only predefined
Scheme functions that might modify stuff get the
`!' treatment.  Consistent application of such
a convention REALLY!WOULD!RESULT!IN!CODE!THAT!
SHOUTS!EVERYWHERE!AND!ALWAYS!!!

If the goal is to forewarn users then such lack of consistency can even have the opposite effect of
lulling them into thinking they're "safe" as long
as they don't see a `!'.  Visible `!'s and hidden
`!'s ... oh my!

Misleading, at best.  Helpful for the first day of
class perhaps, but best abandoned soon thereafter,
after demonstrating the nasty, explosive contagion
of side effects (including structure modification).

Welcome to Lisp, kids.  Procedural, imperative
programming.  Hold onto your seats - things are
about to shake!

Scheme is not Haskell or lambda calculus.  And
Lisp is not Scheme (ducks for cover...).



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-16 17:37   ` Drew Adams
@ 2018-12-16 18:20     ` Stefan Monnier
  2018-12-16 23:06       ` Tom Tromey
  2018-12-17  4:08       ` Stefan Monnier
  2018-12-16 18:21     ` Stefan Monnier
  1 sibling, 2 replies; 20+ messages in thread
From: Stefan Monnier @ 2018-12-16 18:20 UTC (permalink / raw)
  To: Drew Adams; +Cc: Nicolas Petton, Tom Tromey, emacs-devel

I like Scheme's ! (and I don't see in which sense this is "for the
writer" rather than "for the reader").

But I don't actually care.
So really than complain, please provide constructive alternatives.


        Stefan



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-16 17:37   ` Drew Adams
  2018-12-16 18:20     ` Stefan Monnier
@ 2018-12-16 18:21     ` Stefan Monnier
  1 sibling, 0 replies; 20+ messages in thread
From: Stefan Monnier @ 2018-12-16 18:21 UTC (permalink / raw)
  To: Drew Adams; +Cc: Nicolas Petton, Tom Tromey, emacs-devel


And BTW, could someone actually read what I wrote and give comments
about it, rather than about the previous already installed patch?


        Stefan



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-16 18:20     ` Stefan Monnier
@ 2018-12-16 23:06       ` Tom Tromey
  2018-12-17  3:16         ` Stefan Monnier
  2018-12-17  4:08       ` Stefan Monnier
  1 sibling, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2018-12-16 23:06 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Nicolas Petton, Tom Tromey, Drew Adams, emacs-devel

Stefan> So really than complain, please provide constructive alternatives.

I thought my alternative was clear, but FAOD it is: just don't use "!"
as a suffix.

Tom



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-16 23:06       ` Tom Tromey
@ 2018-12-17  3:16         ` Stefan Monnier
  0 siblings, 0 replies; 20+ messages in thread
From: Stefan Monnier @ 2018-12-17  3:16 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Nicolas Petton, Drew Adams, emacs-devel

> Stefan> So really than complain, please provide constructive alternatives.
> I thought my alternative was clear, but FAOD it is: just don't use "!"
> as a suffix.

"don't use "!" as a suffix" doesn't say what to do instead.


        Stefan



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-16 18:20     ` Stefan Monnier
  2018-12-16 23:06       ` Tom Tromey
@ 2018-12-17  4:08       ` Stefan Monnier
  2018-12-17 11:41         ` Nicolas Petton
  1 sibling, 1 reply; 20+ messages in thread
From: Stefan Monnier @ 2018-12-17  4:08 UTC (permalink / raw)
  To: Drew Adams; +Cc: Nicolas Petton, Tom Tromey, emacs-devel

> So really than complain, please provide constructive alternatives.

Or just go and rename it directly.


        Stefan



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-14 17:32 map-put! and (setf (map-elt ...) ..) on lists Stefan Monnier
  2018-12-16 16:32 ` Tom Tromey
@ 2018-12-17 11:38 ` Nicolas Petton
  1 sibling, 0 replies; 20+ messages in thread
From: Nicolas Petton @ 2018-12-17 11:38 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

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

Hi Stefan,

> The current handling of map-put on lists is very ad-hoc:
> The gv-expander of `map-elt` tests if the arg is a list and if so
> delegates to `alist-get`.
>
> It kind of works, but for a library that's supposed to be generic and
> expandable to other map types, this is undesirable.
>
> So in the patch below I change this such that `map-elt` does not special
> case lists any more.  Instead `map-put!` is changed to signal a special
> error when it can't do its job, and the gv-expander of `map-elt` catches
> this error and delegates the job to a new non-side-effecting
> `map-insert`.
>
> With this, we can add new map types via defmethod that work like lists
> (i.e. that don't support inplace update but can still be modified via
> `setf`).
>
> WDYT?

I think it's a good step forward, thank you.

I'd like to start thinking about adding support for plists in map.el.

nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-17  4:08       ` Stefan Monnier
@ 2018-12-17 11:41         ` Nicolas Petton
  2018-12-17 16:06           ` Eli Zaretskii
  2018-12-17 16:07           ` Drew Adams
  0 siblings, 2 replies; 20+ messages in thread
From: Nicolas Petton @ 2018-12-17 11:41 UTC (permalink / raw)
  To: Stefan Monnier, Drew Adams; +Cc: Tom Tromey, emacs-devel

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> So really than complain, please provide constructive alternatives.
>
> Or just go and rename it directly.

Given that seq.el is completely free of side-effects (IIRC, there might
be exceptions but I don't think so), and that map.el was done mimicking
seq.el but for key/value collections, I like the idea of stating clearly
when a function of map.el mutates the argument.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-17 11:41         ` Nicolas Petton
@ 2018-12-17 16:06           ` Eli Zaretskii
  2018-12-17 16:07           ` Drew Adams
  1 sibling, 0 replies; 20+ messages in thread
From: Eli Zaretskii @ 2018-12-17 16:06 UTC (permalink / raw)
  To: Nicolas Petton; +Cc: tom, monnier, drew.adams, emacs-devel

> From: Nicolas Petton <nicolas@petton.fr>
> Date: Mon, 17 Dec 2018 12:41:28 +0100
> Cc: Tom Tromey <tom@tromey.com>, emacs-devel@gnu.org
> 
> Given that seq.el is completely free of side-effects (IIRC, there might
> be exceptions but I don't think so), and that map.el was done mimicking
> seq.el but for key/value collections, I like the idea of stating clearly
> when a function of map.el mutates the argument.

A function whose name begins with "set-" usually does.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-17 11:41         ` Nicolas Petton
  2018-12-17 16:06           ` Eli Zaretskii
@ 2018-12-17 16:07           ` Drew Adams
  2018-12-18 10:11             ` Nicolas Petton
  1 sibling, 1 reply; 20+ messages in thread
From: Drew Adams @ 2018-12-17 16:07 UTC (permalink / raw)
  To: Nicolas Petton, Stefan Monnier; +Cc: Tom Tromey, emacs-devel

> >> So really than complain, please provide constructive alternatives.
> >
> > Or just go and rename it directly.
> 
> Given that seq.el is completely free of side-effects (IIRC, there might
> be exceptions but I don't think so), and that map.el was done mimicking
> seq.el but for key/value collections, I like the idea of stating
> clearly when a function of map.el mutates the argument.

Only when the same function exists in both "destructive"
and non-destructive versions - such as `reverse' and
`nreverse', should we bother to reflect that difference
in the function names - somehow.  And AFAIK we do not
have a convention for doing that.  (Some old Lisp functions
use prefix `n', but that's not great.)

Do we have a non-destructive `map-put'?  I see that there
is a macro `map-put', so I guess that would qualify.  But
no - that macro is now obsolete - replaced by `map-put!'.

Much more important than the function name, however, is
the real answer: Put the important info in the doc string.

That's what should have been done at the outset, and
AFAICT it still hasn't been done, here.  I see these doc
strings for the obsolete `map-put' and the new `map-put!',
respectively:

map-put:

 Associate KEY with VALUE in MAP and return VALUE.
 If KEY is already present in MAP, replace the associated value
 with VALUE.
 When MAP is a list, test equality with TESTFN if non-nil, otherwise use `eql'.

 MAP can be a list, hash-table or array.

map-put!:

 Associate KEY with VALUE in MAP and return VALUE.
 If KEY is already present in MAP, replace the associated value
 with VALUE.

The only change was to get rid of the last two lines (the
first of which is too long, BTW).

Whoa!  Nothing anywhere there that says whether a copy is
returned or the original value is mutated.  That's a bug,
IMO.

If you really want clarity about whether a function is
destructive, put it in the doc string.  That's the real
answer to Stefan's quandary, "Don't use "!" as a suffix"
doesn't say what to do instead."

That's the place to start.  And unless there is also to
be a non-destructive version, and we _need_ to have two
different names, there is no need to have any name
indication that a given function is "destructive".

(Just one opinion.)



^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-17 16:07           ` Drew Adams
@ 2018-12-18 10:11             ` Nicolas Petton
  2018-12-18 13:56               ` Stefan Monnier
  2018-12-18 15:34               ` Drew Adams
  0 siblings, 2 replies; 20+ messages in thread
From: Nicolas Petton @ 2018-12-18 10:11 UTC (permalink / raw)
  To: Drew Adams, Stefan Monnier; +Cc: Tom Tromey, emacs-devel

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]

Drew Adams <drew.adams@oracle.com> writes:

> Do we have a non-destructive `map-put'?

There's `map-insert'.

> I see that there is a macro `map-put', so I guess that would qualify.
> But no - that macro is now obsolete - replaced by `map-put!'.
>
> Much more important than the function name, however, is
> the real answer: Put the important info in the doc string.

The docstring, yes, of course, but the function name is IMHO the most
important part of the documentation.

> That's what should have been done at the outset, and
> AFAICT it still hasn't been done, here.  I see these doc
> strings for the obsolete `map-put' and the new `map-put!',
> respectively:
>
> map-put:
>
>  Associate KEY with VALUE in MAP and return VALUE.
>  If KEY is already present in MAP, replace the associated value
>  with VALUE.
>  When MAP is a list, test equality with TESTFN if non-nil, otherwise use `eql'.
>
>  MAP can be a list, hash-table or array.
>
> map-put!:
>
>  Associate KEY with VALUE in MAP and return VALUE.
>  If KEY is already present in MAP, replace the associated value
>  with VALUE.
>
> The only change was to get rid of the last two lines (the
> first of which is too long, BTW).
>
> Whoa!  Nothing anywhere there that says whether a copy is
> returned or the original value is mutated.  That's a bug,
> IMO.

Agreed.  I'll fix that.

> If you really want clarity about whether a function is
> destructive, put it in the doc string.  That's the real
> answer to Stefan's quandary, "Don't use "!" as a suffix"
> doesn't say what to do instead."
>
> That's the place to start.  And unless there is also to
> be a non-destructive version, and we _need_ to have two
> different names, there is no need to have any name
> indication that a given function is "destructive".

As I said, I like Stefan's approach, but if everybody else dislike it,
and if you think it's not idiomatic Elisp, then let's change the name.

Stefan, would it be ok with you?

Cheers,
Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-18 10:11             ` Nicolas Petton
@ 2018-12-18 13:56               ` Stefan Monnier
  2018-12-18 15:42                 ` Drew Adams
  2018-12-18 15:34               ` Drew Adams
  1 sibling, 1 reply; 20+ messages in thread
From: Stefan Monnier @ 2018-12-18 13:56 UTC (permalink / raw)
  To: Nicolas Petton; +Cc: Tom Tromey, Drew Adams, emacs-devel

>> map-put!:
>>
>>  Associate KEY with VALUE in MAP and return VALUE.
>>  If KEY is already present in MAP, replace the associated value
>>  with VALUE.

Not sure which version you were looking at, but what I see is:

      "Associate KEY with VALUE in MAP.
    If KEY is already present in MAP, replace the associated value
    with VALUE.
    This operates by modifying MAP in place.
    If it cannot do that, it signals the `map-not-inplace' error.
    If you want to insert an element without modifying MAP, use `map-insert'."

> As I said, I like Stefan's approach, but if everybody else dislike it,
> and if you think it's not idiomatic Elisp, then let's change the name.
>
> Stefan, would it be ok with you?

As I said, I like Scheme's ! convention, but I really don't care very
much about the name: I think normal code should never call this function
directly and should use `(setf (map-elt ...) ..)` instead anyway!
So, even a longwinded name would be perfectly fine.


        Stefan



^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-18 10:11             ` Nicolas Petton
  2018-12-18 13:56               ` Stefan Monnier
@ 2018-12-18 15:34               ` Drew Adams
  2018-12-18 15:47                 ` Stefan Monnier
  2018-12-18 16:34                 ` Nicolas Petton
  1 sibling, 2 replies; 20+ messages in thread
From: Drew Adams @ 2018-12-18 15:34 UTC (permalink / raw)
  To: Nicolas Petton, Stefan Monnier; +Cc: Tom Tromey, emacs-devel

> > Do we have a non-destructive `map-put'?
> 
> There's `map-insert'.

My point was that there is no need for a function
name to signal that the function so named is
"destructive".  The doc string should do that,
however.

The only case where we might need a function name
to signal this is when we have two functions with
exactly the same name otherwise, one "destructive"
and the other not.

There is macro `map-put', which is supposedly
obsolete now.  It would be fine (IMHO) to just
name the new, "destructive" function `map-put',
IOW replace the macro that is anyway obsolete.

If we did that we'd need to post a notice (in
NEWS and the doc string, for example) that we've
made such an incompatible change.  Probably not
a big deal.

> > Much more important than the function name, however, is
> > the real answer: Put the important info in the doc string.
> 
> The docstring, yes, of course, but the function name is IMHO the most
> important part of the documentation.

The function name should, yes, say what the function
does.  But it need not always say how it does it or
whether the action might change list structure etc.

Or if it does then that should be done systematically,
for the names of all "destructive" functions.  It is
misleading in Lisp to have some "destructive" functions
signal that fact in their names, and others not do so.

And as I said, if you use such a function in a function
you define, it can often be the case that your function
is, likewise, "destructive". Are you going to name your
function using such a name signal too?  And so on.  It's
a contagious virus, which we generally ignore, in terms
of propagating such a name convention.

Again, just one, no doubt minority, opinion.  I express
the opinion, but I don't expect that it will have much
effect.
 
> As I said, I like Stefan's approach, but if everybody else dislike it,
> and if you think it's not idiomatic Elisp, then let's change the name.
> 
> Stefan, would it be ok with you?

I don't think there is a standard idiom.  Some names
indicate (if a user knows the particular convention)
possible "destruction", but others do not.

An old user might recognize that `nreverse' is a
"destructive" `reverse' from the name, but a new user
might not.  The doc should be the place where we make
very clear what a function does and whether it is
"destructive".

If Emacs does want to follow a convention of naming
"destructive" functions then it should do so more
systematically, IMO.  (And IMO it should not bother
to do so.)  In that case, apply the convention all
over the place, even including renaming (but keeping
an old alias) functions such as `nreverse'.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-18 13:56               ` Stefan Monnier
@ 2018-12-18 15:42                 ` Drew Adams
  0 siblings, 0 replies; 20+ messages in thread
From: Drew Adams @ 2018-12-18 15:42 UTC (permalink / raw)
  To: Stefan Monnier, Nicolas Petton; +Cc: Tom Tromey, emacs-devel

> >> map-put!:
> >>  Associate KEY with VALUE in MAP and return VALUE.
> >>  If KEY is already present in MAP, replace the associated value
> >>  with VALUE.
> 
> Not sure which version you were looking at,

The version I downloaded from Master on 2019-12-17,
the same day I wrote that.

(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 value)
              (error "No place to change the mapping for %S" key)))
    :hash-table (puthash key value map)
    :array (aset map key value)))

> but what I see is:
>       "Associate KEY with VALUE in MAP.
>     If KEY is already present in MAP, replace the associated value
>     with VALUE.
>     This operates by modifying MAP in place.
>     If it cannot do that, it signals the `map-not-inplace' error.
>     If you want to insert an element without modifying MAP, use `map-
>     insert'."

LGTM.  Thanks for making that change/addition.

> I think normal code should never call this function
> directly and should use `(setf (map-elt ...) ..)` instead anyway!
> So, even a longwinded name would be perfectly fine.

1. Maybe such a statement should be made somewhere for
users, if it's not made already - and with maybe some
description of what is meant by either "normal code"
or (more likely) the abnormal code where it might make
sense to use this.

2. +1 for a long-winded name, if Emacs wants to make
the name proclaim that the function is destructive.
Maybe something like `map-put-in-place' or
`in-place-map-put' or `map-put-set' or `map-put-modify'
(no, I don't have a great suggestion).



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-18 15:34               ` Drew Adams
@ 2018-12-18 15:47                 ` Stefan Monnier
  2018-12-18 16:34                 ` Nicolas Petton
  1 sibling, 0 replies; 20+ messages in thread
From: Stefan Monnier @ 2018-12-18 15:47 UTC (permalink / raw)
  To: Drew Adams; +Cc: Nicolas Petton, Tom Tromey, emacs-devel

> My point was that there is no need for a function
> name to signal that the function so named is
> "destructive".  The doc string should do that, however.

I disagree.  It doesn't have to be something like ! but it shouldn't be
necessary to look at the docstring to figure that out.


        Stefan



^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-18 15:34               ` Drew Adams
  2018-12-18 15:47                 ` Stefan Monnier
@ 2018-12-18 16:34                 ` Nicolas Petton
  2018-12-18 17:41                   ` Drew Adams
  1 sibling, 1 reply; 20+ messages in thread
From: Nicolas Petton @ 2018-12-18 16:34 UTC (permalink / raw)
  To: Drew Adams, Stefan Monnier; +Cc: Tom Tromey, emacs-devel

[-- Attachment #1: Type: text/plain, Size: 558 bytes --]

Drew Adams <drew.adams@oracle.com> writes:

>> > Do we have a non-destructive `map-put'?
>> 
>> There's `map-insert'.
>
> My point was that there is no need for a function
> name to signal that the function so named is
> "destructive".  The doc string should do that,
> however.

It can be very important to know that a function is destructive, I don't
think it's a should be seen as an internal detail of the function.  IOW,
I think that whether a function is destructive or not is part of what
the function does, not how it does it.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-18 16:34                 ` Nicolas Petton
@ 2018-12-18 17:41                   ` Drew Adams
  2018-12-18 20:44                     ` Nicolas Petton
  0 siblings, 1 reply; 20+ messages in thread
From: Drew Adams @ 2018-12-18 17:41 UTC (permalink / raw)
  To: Nicolas Petton, Stefan Monnier; +Cc: Tom Tromey, emacs-devel

> > My point was that there is no need for a function
> > name to signal that the function so named is
> > "destructive".  The doc string should do that,
> > however.
> 
> It can be very important to know that a function is destructive, I don't
> think it's a should be seen as an internal detail of the function.  IOW,
> I think that whether a function is destructive or not is part of what
> the function does, not how it does it.

It's not an internal detail - agreed.  It's
an important part of what it does (can do).
Very important.

And yet you don't know such behavior from
99.9999% of the existing function names,
do you?

When you write a function that makes use of a
destructive function, and whose action might
itself change something, including, e.g.,
list structure, do you name it in a special
way?  Do you think that others do so?

You can't rely on such naming, because it
Just. Doesn't. Happen.

And if it did happen then such name suffixes
would be all over the place - more noise than
signal - like ignored legalistic warnings
that serve only to protect the warners from
lawsuits.

Much such rightfully gets relegated to the
fine print.  Can we guess why?  Because it's
noise, not signal.  How does it happen that
something _very important_ can become noise?
Too much of it.  When you are warned about
everything you are really not warned about
anything.

Lisp is an imperative, procedural language.
Like that or hate it; it's a fact.  If you
want a declarative language, Lisp ain't it.

One might argue that there are different
degrees or kinds of state modification /
mutation, and thus different sorts of "danger".
Maybe only some kinds merit a DANGER! suffix?

Perhaps you feel that `setq' and `set' are
not sufficiently dangerous to merit renaming
to `setq!' and `set!' (oops, the latter is
Scheme's `setq', it doesn't correspond to
Lisp's `set').  Or perhaps not. `setf!'?
`put!'?  `push!'? `load-library!'?  On and
on...

Or perhaps you'd propose we reserve `!'
for (possible) list modification? sequence
modification?  Something else?

If we're going to have a naming convention
then hey, let's really have a convention.
Seriously.

Regardless of whether we decide to rename
existing functions, if we're going to have
a convention then we should apply it for
everything new.

And we should document it for users,
encouraging them too to respect it.
Otherwise users will fall into pitfalls -
unexpected (because unwarned) gotchas.
Truth in labeling, after all.

Do I think we should have such a convention?
Nope.  Do I think we should use suffix `!'
just here and there, for only a few new
functions?  Nope.  Do I think we should call
`map-put' `map-put!' because it can modify
list structure, hash tables, arrays...?
Nope.

The choices really are these, I think:

1. Put up one big general sign saying,
   "Lisp may be hazardous to your health."

   Oops, I mean, "Lisp is an imperative,
   procedural language.  Be aware that data
   mutation and other state CHANGE HAPPENS."

2. Rigorously put warning signs on everything
   that might modify state, which means
   pretty much everything that might come
   into contact with something that might
   modify state, which means most everything.

I vote for #1.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: map-put! and (setf (map-elt ...) ..) on lists
  2018-12-18 17:41                   ` Drew Adams
@ 2018-12-18 20:44                     ` Nicolas Petton
  0 siblings, 0 replies; 20+ messages in thread
From: Nicolas Petton @ 2018-12-18 20:44 UTC (permalink / raw)
  To: Drew Adams, Stefan Monnier; +Cc: Tom Tromey, emacs-devel

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

Drew Adams <drew.adams@oracle.com> writes:

Hi Drew,

> If we're going to have a naming convention
> then hey, let's really have a convention.
> Seriously.
>
> Regardless of whether we decide to rename
> existing functions, if we're going to have
> a convention then we should apply it for
> everything new.

Yes, I agree with that, and I since wouldn't want to have it as a
convention because it would be very hard if not impossible to do, I
think it's better to just rename `map-put!'.

Cheers,
Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2018-12-18 20:44 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-14 17:32 map-put! and (setf (map-elt ...) ..) on lists Stefan Monnier
2018-12-16 16:32 ` Tom Tromey
2018-12-16 17:37   ` Drew Adams
2018-12-16 18:20     ` Stefan Monnier
2018-12-16 23:06       ` Tom Tromey
2018-12-17  3:16         ` Stefan Monnier
2018-12-17  4:08       ` Stefan Monnier
2018-12-17 11:41         ` Nicolas Petton
2018-12-17 16:06           ` Eli Zaretskii
2018-12-17 16:07           ` Drew Adams
2018-12-18 10:11             ` Nicolas Petton
2018-12-18 13:56               ` Stefan Monnier
2018-12-18 15:42                 ` Drew Adams
2018-12-18 15:34               ` Drew Adams
2018-12-18 15:47                 ` Stefan Monnier
2018-12-18 16:34                 ` Nicolas Petton
2018-12-18 17:41                   ` Drew Adams
2018-12-18 20:44                     ` Nicolas Petton
2018-12-16 18:21     ` Stefan Monnier
2018-12-17 11:38 ` Nicolas Petton

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).