Re: Replace trivial pcase occurrences in the Emacs sources

["Garreau\, Alexandre" <galex-713@galex-713.eu>]

On 2018-10-29 at 17:08, Stefan Monnier wrote:
>>> > Doc strings which specify fully the arguments to these macros, including
>>> > their semantics, and say what the macros do.  The current doc strings
>>> > (at least some of them) for these macros don't do this.
>>> I understand this in theory, but I don't know what it means in this
>>> concrete case.
>> Take a look at, for example, the doc string for pcase-dolist.  In its
>> entirety, it's this:
>
> Thanks.  What do you think of the patch below?
>
> I'd rather keep it defined in terms of its differences w.r.t `dolist`,
> but if really needed, we could change the doc so it doesn't rely on
> `dolist`s own doc at all.

You can do that while being more explicit on how arguments relate.  A
little redundancy might be welcome, but I believe the standard minimum
(that, for instance, would satisfy checkdoc) would only to cite each
argument name in upcase and say it is like in `dolist', only if this is
*exactely the case*.  It is not for “PATTERN” so usage ought to be more
detailed: instead of describing what is done, you’d better explain what
it is for.  So in “More specifically `dolist's VAR binding is replaced
by a PATTERN against which each element of the list is matched.” you’re
simply describing your macro implementation, which readers doesn’t want
(otherwise they would be reading function’s source), you’d better say
“PATTERN allow to bind several variables at once through a `pcase'
pattern, instead of only one var as in simple `dolist'”.  This is way
clearer (I really ought to learn how to make up patches ><).

> We do have to keep the reference to `pcase` because we don't want to
> repeat the definition of what a pcase pattern can look like.

Of course.  But you want to precise how exactely does it relate to
`pcase'.

>  (defmacro pcase-let (bindings &rest body)
>    "Like `let' but where you can use `pcase' patterns for bindings.
>  BODY should be a list of expressions, and BINDINGS should be a list of bindings
> -of the form (PAT EXP).
> +of the form (PATTERN EXP).

So `pcase-let' doesn’t support single variable names bound to `nil' (not
that sad, I (sadly) never use that anyway, that’s only for lexical
locality tweaking)?  I checked, in fact it indeed doesn’t: knowing the
other `pcase-' functions docstrings tendency to omit optional behavior,
if I didn’t checked, I’d have bet it were supporting it.

Then, contrarily to `let' you might make your docstring more readable by
changing the arglist spec: "\(fn ((PATTERN EXP)...) BODY...)"

or you may be more formal, so it’d become more readable: “BINDINGS is a
list of the form (BINDING...), with each BINDING is (PATTERN EXP), where
PATTERN is a `pcase' pattern, and EXP the expression it must match.”

>  ;;;###autoload
>  (defmacro pcase-dolist (spec &rest body)
> -  "Like `dolist' but where the binding can be a `pcase' pattern.
> +  "Superset of `dolist' where the VAR binding can be a `pcase' PATTERN.
> +More specifically `dolist's VAR binding is replaced by a PATTERN
> +against which each element of the list is matched.
> +As in the case of `pcase-let', PATTERN is matched under the assumption
> +that it *will* match.

First I don’t believe it is a good idea to talk about `pcase-let' in the
docstring of another function, or then you should cite the whole family
(including `pcase-lambda', etc.), except if they’re tightly tied, but
then you should also cite `pcase-dolist' in `pcase-let' docstring, so to
make a followable dual-link.

>  The macro is expanded and optimized under the assumption that those
>  patterns *will* match, so a mismatch may go undetected or may cause
>  any kind of error."

That’s not explicit enough on the result if not doing so: what kind of
error?

From what I see there:

#+BEGIN_SRC emacs-lisp
(pcase-let ((`(,a ,b ,c) (list 1 2))) (list a b c)) ; => (1 2 nil)

(pcase-let ((`(,a ,b (2 3)) (list 1 2 3))) (list a b))
;; => (wrong-type-argument listp 3)

(pcase-let ((`(,a ,b (2 3)) (list 1 2 (list 4 5)))) (list a b))
;; => (1 2)
#+END_SRC

It seems to be like `pcase-lambda': it binds nil, and doesn’t err
anything, as long as the structures are in the same places and the same
types.

This behavior ought to be documented, or changed.

In my message about `pcase-lambda', I wrote:
> A given subpattern inside PATTERN might not match values (equality) or
> even types, except for each of its subsequences, which *have* to
> exist, have the right type, and be in the right place.

>  \n(fn (PATTERN LIST) BODY...)"
>    (declare (indent 1) (debug ((pcase-PAT form) body)))
>    (if (pcase--trivial-upat-p (car spec))

So `pcase-dolist' doesn’t support `dolist'’s `result'?  I bet it does
(if it doesn’t it’s sad), and like in `pcase-lambda' you again omitted
optional arguments from your docstring: this is confusing, as you might
as well have implemented it so that to remove or override these.