Thank you for the review! Please, see my answers inline and an updated patch attached. On Fri, Jul 29, 2016 at 4:10 AM, wrote: > immerrr again 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