bug#50965: Allow nil third argument for mapconcat

[Stefan Kangas <stefan@marxist.se>]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> +Optional argument SEPARATOR must be a string, a vector, or a list of
>> +characters; nil is the empty string."
>
> nil is not a string, so saying that is not the best wording.  I
> suggest "nil stands for an empty string." instead.

Yes, that's better.  Fixed.

>> +(ert-deftest fns-tests-mapconcat ()
>> +  (should (string= (mapconcat #'identity '() "_") ""))
>> +  (should (string= (mapconcat #'identity '("A") "_") "A"))
>> +  (should (string= (mapconcat #'identity '("A" "B") "_") "A_B"))
>> +  (should (string= (mapconcat #'identity '("A" "B" "C") "_") "A_B_C"))
>> +  (should (string= (mapconcat #'identity '("a" "b")) "ab")))
>
> Please be sure to test with vectors, bool-vectors, list of characters,
> and non-ASCII strings as well.

I've added such tests.  Please check especially the bool-vector case,
as I was unsure about what exactly to test for.  I put this:

    (should (string= (mapconcat #'identity [nil nil] "") ""))
    (should-error (mapconcat #'identity [nil nil t])
                  :type 'wrong-type-argument)

>> I'm assuming this should go to master at this point.
>
> Yes, thanks.

New patch attached.

[-- Attachment #2: 0001-Make-mapconcat-argument-separator-optional.patch --]
[-- Type: text/x-diff, Size: 5918 bytes --]

From 7646c687a899f8bba7e71c6f5f59f4262bb7d592 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Sun, 3 Oct 2021 19:24:31 +0200
Subject: [PATCH] Make 'mapconcat' argument 'separator' optional

* src/fns.c (Fmapconcat): Make third 'separator' argument
optional.  (Bug#50965)
* doc/lispref/functions.texi (Mapping Functions): Update
documentation for above change.
* test/src/fns-tests.el (fns-tests-mapconcat): New test.

* doc/misc/cl.texi (Obsolete Setf Customization): Don't use third
mapconcat argument in example.
* lisp/emacs-lisp/subr-x.el (string-join): Doc fix.
---
 doc/lispref/functions.texi |  9 ++++-----
 doc/misc/cl.texi           |  2 +-
 etc/NEWS                   |  4 ++++
 lisp/emacs-lisp/subr-x.el  |  4 +++-
 src/fns.c                  |  8 ++++++--
 test/src/fns-tests.el      | 17 +++++++++++++++++
 6 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/doc/lispref/functions.texi b/doc/lispref/functions.texi
index c856557c3c..3163300184 100644
--- a/doc/lispref/functions.texi
+++ b/doc/lispref/functions.texi
@@ -961,14 +961,14 @@ Mapping Functions
 into a list.  @code{mapc} always returns @var{sequence}.
 @end defun
 
-@defun mapconcat function sequence separator
+@defun mapconcat function sequence &optional separator
 @code{mapconcat} applies @var{function} to each element of
 @var{sequence}; the results, which must be sequences of characters
 (strings, vectors, or lists), are concatenated into a single string
 return value.  Between each pair of result sequences, @code{mapconcat}
 inserts the characters from @var{separator}, which also must be a
-string, or a vector or list of characters.  @xref{Sequences Arrays
-Vectors}.
+string, or a vector or list of characters; a @code{nil} value is
+treated as the empty string.  @xref{Sequences Arrays Vectors}.
 
 The argument @var{function} must be a function that can take one
 argument and returns a sequence of characters: a string, a vector, or
@@ -986,8 +986,7 @@ Mapping Functions
 
 @group
 (mapconcat (lambda (x) (format "%c" (1+ x)))
-           "HAL-8000"
-           "")
+           "HAL-8000")
      @result{} "IBM.9111"
 @end group
 @end example
diff --git a/doc/misc/cl.texi b/doc/misc/cl.texi
index a6c3c32c0e..f31dedae88 100644
--- a/doc/misc/cl.texi
+++ b/doc/misc/cl.texi
@@ -5024,7 +5024,7 @@ Obsolete Setf Customization
 @ignore
 (defmacro concatf (place &rest args)
   (gv-letplace (getter setter) place
-    (macroexp-let2 nil v (mapconcat 'identity args "")
+    (macroexp-let2 nil v (mapconcat 'identity args)
       (funcall setter `(concat ,getter ,v)))))
 @end ignore
 @end defmac
diff --git a/etc/NEWS b/etc/NEWS
index 20ed20308e..afd335163b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -80,6 +80,10 @@ Emacs buffers, like indentation and the like.  The new ert function
 \f
 * Lisp Changes in Emacs 29.1
 
++++
+** Third 'mapconcat' argument 'separator' is now optional.
+An explicit nil always meant the empty string, now it can be left out.
+
 ---
 ** Themes can now be made obsolete.
 Using 'make-obsolete' on a theme is now supported.  This will make
diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 91ebbf9fb9..7d1b6d85ed 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -208,7 +208,9 @@ string-empty-p
   (string= string ""))
 
 (defsubst string-join (strings &optional separator)
-  "Join all STRINGS using SEPARATOR."
+  "Join all STRINGS using SEPARATOR.
+Optional argument SEPARATOR must be a string, a vector, or a list of
+characters; nil stands for the empty string."
   (mapconcat #'identity strings separator))
 
 (define-obsolete-function-alias 'string-reverse 'reverse "25.1")
diff --git a/src/fns.c b/src/fns.c
index a72e41aee5..61182852ba 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2852,12 +2852,16 @@ mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
   return leni;
 }
 
-DEFUN ("mapconcat", Fmapconcat, Smapconcat, 3, 3, 0,
+DEFUN ("mapconcat", Fmapconcat, Smapconcat, 2, 3, 0,
        doc: /* Apply FUNCTION to each element of SEQUENCE, and concat the results as strings.
 In between each pair of results, stick in SEPARATOR.  Thus, " " as
   SEPARATOR results in spaces between the values returned by FUNCTION.
+
 SEQUENCE may be a list, a vector, a bool-vector, or a string.
-SEPARATOR must be a string, a vector, or a list of characters.
+
+Optional argument SEPARATOR must be a string, a vector, or a list of
+characters; nil stands for the empty string.
+
 FUNCTION must be a function of one argument, and must return a value
   that is a sequence of characters: either a string, or a vector or
   list of numbers that are valid character codepoints.  */)
diff --git a/test/src/fns-tests.el b/test/src/fns-tests.el
index 5759457209..2d641cc311 100644
--- a/test/src/fns-tests.el
+++ b/test/src/fns-tests.el
@@ -430,6 +430,23 @@ fns-tests-hash-buffer
                    (buffer-hash))
                  (sha1 "foo"))))
 
+(ert-deftest fns-tests-mapconcat ()
+  (should (string= (mapconcat #'identity '()) ""))
+  (should (string= (mapconcat #'identity '("a" "b")) "ab"))
+  (should (string= (mapconcat #'identity '() "_") ""))
+  (should (string= (mapconcat #'identity '("A") "_") "A"))
+  (should (string= (mapconcat #'identity '("A" "B") "_") "A_B"))
+  (should (string= (mapconcat #'identity '("A" "B" "C") "_") "A_B_C"))
+  ;; non-ASCII strings
+  (should (string= (mapconcat #'identity '("Ä" "ø" "☭" "தமிழ்") "_漢字_")
+                   "Ä_漢字_ø_漢字_☭_漢字_தமிழ்"))
+  ;; vector
+  (should (string= (mapconcat #'identity ["a" "b"] "") "ab"))
+  ;; bool-vector
+  (should (string= (mapconcat #'identity [nil nil] "") ""))
+  (should-error (mapconcat #'identity [nil nil t])
+                :type 'wrong-type-argument))
+
 (ert-deftest fns-tests-mapcan ()
   (should-error (mapcan))
   (should-error (mapcan #'identity))
-- 
2.30.2