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

[immerrr again]

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

0001-Fix-regexp-opt-documentation-bug-17862.patch
Description: Text Data