bug#17862: 24.3; regexp-opt docstring is incorrect

[immerrr again <immerrr@gmail.com>]

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

Thank you for the review!

Please, see my answers inline and an updated patch attached.

On Fri, Jul 29, 2016 at 4:10 AM,  <npostavs@users.sourceforge.net> wrote:
> immerrr again <immerrr@gmail.com> writes:
>
>> +non-@code{nil}
>
> Should be just non-nil, I believe.
>

I was following the precedent set by other documentation in searching.texi.

>> +    the resulting regexp is surrounded by @samp{\(} and @samp{\)}.
>> +
>> +Otherwise the resulting regexp is surrounded by @samp{\(?:} and
>> +@samp{\)}.
>
> This is not 100% accurate, here is a counterexample:
>
>     (regexp-opt '("a" "b" "c")) ;=> "[abc]"
>
> Not sure how detailed we want to be about this is in the docs though.
>

Agreed, I think reference documentation must cover all cases.  I have updated
it.  The description still seems concise but might have become confusing and
might need an illustrative example like yours.

>>
>>  This simplified definition of @code{regexp-opt} produces a
>>  regular expression which is equivalent to the actual value
>> -(but not as efficient):
>> +(but typically more efficient):
>
> "not as efficient" refers to the output of the simplified definition.
>

Right.  The paragraph referring to "regexp-opt" got me confused, I have
renamed the simplified version "simplified-regexp-opt" for clarity.

>>
>>  @example
>>  (defun regexp-opt (strings &optional paren)
>> -  (let ((open-paren (if paren "\\(" ""))
>> -        (close-paren (if paren "\\)" "")))
>> +  (let ((open-paren (make-open-paren paren))
>> +        (close-paren (make-close-paren paren)))
>
> I'm not sure that adding these undefined make-{open,close}-paren
> functions makes things any clearer.
>

I agree that adding "impenetrable definitions" might be unclear, but I'd argue
that it's still an improvement because the equivalence statement is
plain false now:

- "words" and "symbol" keywords add special backslash characters
around the result

- if PAREN is nil, the result is still grouped (unless the strings can be
  represented as a character set, but that's not handled in the
simplified version)

I doubt we can put all the logic behind PAREN into the simplified version.  I
made an attempt to clarify the "impenetrable definitions" by their name, but if
that's still unacceptable we might want to drop the "is equivalent to" part
which is currently misleading.

>>  The returned regexp is typically more efficient than the equivalent regexp:
>>
>> - (let ((open (if PAREN \"\\\\(\" \"\")) (close (if PAREN \"\\\\)\" \"\")))
>> + (let ((open (make-open-paren PAREN)) (close (make-close-paren PAREN)))
>
> Same comment here.

Same response here :)

Cheers,
immerrr

[-- Attachment #2: 0001-Fix-regexp-opt-documentation-bug-17862.patch --]
[-- Type: text/x-patch, Size: 5045 bytes --]

From 1cf5026b56b6b1b58f8401c7642d06a2809f1e65 Mon Sep 17 00:00:00 2001
From: immerrr <immerrr@gmail.com>
Date: Sun, 7 Feb 2016 12:46:37 +0300
Subject: [PATCH] Fix regexp-opt documentation (bug #17862)

* lisp/emacs-lisp/regexp-opt.el (regexp-opt): update PAREN doc

* doc/lispref/searching.texi (Regexp Functions): update PAREN doc
---
 doc/lispref/searching.texi    | 43 ++++++++++++++++++++++++++++---------------
 lisp/emacs-lisp/regexp-opt.el | 41 +++++++++++++++++++++++++++++++----------
 2 files changed, 59 insertions(+), 25 deletions(-)

diff --git a/doc/lispref/searching.texi b/doc/lispref/searching.texi
index 1243d72..b0ad435 100644
--- a/doc/lispref/searching.texi
+++ b/doc/lispref/searching.texi
@@ -946,23 +946,36 @@ Regexp Functions
 more efficient, but is almost never worth the effort.}.
 @c E.g., see http://debbugs.gnu.org/2816
 
-If the optional argument @var{paren} is non-@code{nil}, then the
-returned regular expression is always enclosed by at least one
-parentheses-grouping construct.  If @var{paren} is @code{words}, then
-that construct is additionally surrounded by @samp{\<} and @samp{\>};
-alternatively, if @var{paren} is @code{symbols}, then that construct
-is additionally surrounded by @samp{\_<} and @samp{\_>}
-(@code{symbols} is often appropriate when matching
-programming-language keywords and the like).
-
-This simplified definition of @code{regexp-opt} produces a
-regular expression which is equivalent to the actual value
-(but not as efficient):
+The optional argument @var{paren} can be any of the following:
+
+a string
+    the resulting regexp is preceded by @var{paren} and followed by
+    @samp{\)}, e.g. use @samp{"\\(?1:"} to produce an explicitly
+    numbered group.
+
+@code{words}
+    the resulting regexp is surrounded by @samp{\<\(} and @samp{\)\>}.
+
+@code{symbols}
+    the resulting regexp is surrounded by @samp{\_<\(} and @samp{\)\_>}
+    (this is often appropriate when maching programming-language
+    keywords and the like).
+
+non-@code{nil}
+    the resulting regexp is surrounded by @samp{\(} and @samp{\)}.
+
+@code{nil}
+    if all @var{strings} are single-character, the resulting regexp is
+    not surrounded, otherwise it is surrounded by @samp{\(?:} and
+    @samp{\)}.
+
+The resulting regexp of @code{regexp-opt} is equivalent to but usually
+more efficient than that of a simplified version:
 
 @example
-(defun regexp-opt (strings &optional paren)
-  (let ((open-paren (if paren "\\(" ""))
-        (close-paren (if paren "\\)" "")))
+(defun simplified-regexp-opt (strings &optional paren)
+  (let ((open-paren (open-paren-logic-as-described-above paren))
+        (close-paren (close-paren-logic-as-described-above paren)))
     (concat open-paren
             (mapconcat 'regexp-quote strings "\\|")
             close-paren)))
diff --git a/lisp/emacs-lisp/regexp-opt.el b/lisp/emacs-lisp/regexp-opt.el
index b1e132a..080aa32 100644
--- a/lisp/emacs-lisp/regexp-opt.el
+++ b/lisp/emacs-lisp/regexp-opt.el
@@ -86,18 +86,39 @@
 ;;;###autoload
 (defun regexp-opt (strings &optional paren)
   "Return a regexp to match a string in the list STRINGS.
-Each string should be unique in STRINGS and should not contain any regexps,
-quoted or not.  If optional PAREN is non-nil, ensure that the returned regexp
-is enclosed by at least one regexp grouping construct.
-The returned regexp is typically more efficient than the equivalent regexp:
+Each string should be unique in STRINGS and should not contain
+any regexps, quoted or not.  Optional PAREN specifies how the
+returned regexp is surrounded by grouping constructs.
 
- (let ((open (if PAREN \"\\\\(\" \"\")) (close (if PAREN \"\\\\)\" \"\")))
-   (concat open (mapconcat \\='regexp-quote STRINGS \"\\\\|\") close))
+The optional argument PAREN can be any of the following:
 
-If PAREN is `words', then the resulting regexp is additionally surrounded
-by \\=\\< and \\>.
-If PAREN is `symbols', then the resulting regexp is additionally surrounded
-by \\=\\_< and \\_>."
+a string
+    the resulting regexp is preceded by PAREN and followed by
+    \\), e.g.  use \"\\\\(?1:\" to produce an explicitly numbered
+    group.
+
+`words'
+    the resulting regexp is surrounded by \\=\\<\\( and \\)\\>.
+
+`symbols'
+    the resulting regexp is surrounded by \\_<\\( and \\)\\_>.
+
+non-nil
+    the resulting regexp is surrounded by \\( and \\).
+
+nil
+    if all STRINGS are single-character, the resulting regexp is
+    not surrounded, otherwise it is surrounded by \\(?: and \\).
+
+The resulting regexp is equivalent to but usually more efficient
+than that of a simplified version:
+
+ (defun simplified-regexp-opt (strings &optional paren)
+   (let ((open-paren (open-paren-logic-as-described-above paren))
+         (close-paren (close-paren-logic-as-described-above paren)))
+     (concat open-paren
+             (mapconcat 'regexp-quote strings \"\\\\|\")
+             close-paren)))"
   (save-match-data
     ;; Recurse on the sorted list.
     (let* ((max-lisp-eval-depth 10000)
-- 
2.9.2