Re: master a6b5985: Avoid duplicated character classes in rx

classic Classic list List threaded Threaded
33 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Juanma Barranquero
On Tue, Dec 3, 2019 at 3:23 PM Mattias Engdegård <[hidden email]> wrote:

>              ((and (symbolp arg)
>                    (let ((class (cdr (assq arg rx--char-classes))))
> -                    (and class (push class classes)))))
> +                    (and class
> +                         (or (memq class classes)
> +                             (push class classes))))))

This (which is a branch of a `cond') relies in the fact that (push ELEMENT LISTNAME)
returns the new LISTNAME.

Which isn't really documented. It's sort-of-documented because push's docstring
says that it is "morally equivalent to (setf place (cons newelt place)", and the
elisp manual says that it is "equivalent to (setq ...)" or that it "does the
equivalent of (setf ...)".

Shouldn't we say it in its docstring?

(BTW, push's args are called ELEMENT and LISTNAME in the manual, NEWELT and PLACE
in the code. Frankly, I think they should be ELEMENT and PLACE in both cases.)

Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Stefan Monnier
>>              ((and (symbolp arg)
>>                    (let ((class (cdr (assq arg rx--char-classes))))
>> -                    (and class (push class classes)))))
>> +                    (and class
>> +                         (or (memq class classes)
>> +                             (push class classes))))))
>
> This (which is a branch of a `cond') relies in the fact that (push ELEMENT
> LISTNAME)
> returns the new LISTNAME.
>
> Which isn't really documented. It's sort-of-documented because push's
> docstring
> says that it is "morally equivalent to (setf place (cons newelt place)",
> and the
> elisp manual says that it is "equivalent to (setq ...)" or that it "does the
> equivalent of (setf ...)".
>
> Shouldn't we say it in its docstring?

I'd rather fix the code not to rely on the return value.
E.g.

                 ((and (symbolp arg)
                       (let ((class (cdr (assq arg rx--char-classes))))
    -                    (and class (push class classes)))))
    +                    (and class
    +                         (progn (cl-pushnew class classes) t))))


-- Stefan


Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Mattias Engdegård-2
3 dec. 2019 kl. 16.26 skrev Stefan Monnier <[hidden email]>:

> I'd rather fix the code not to rely on the return value.

I'm sure a lot more code relies on the return value of 'push'.
It's also well-defined in Common Lisp. Why not just document it?


Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Juanma Barranquero
In reply to this post by Stefan Monnier

On Tue, Dec 3, 2019 at 4:26 PM Stefan Monnier <[hidden email]> wrote:

> I'd rather fix the code not to rely on the return value.

Agreed, but I wasn't talking specifically about this case. There are others in the code,
I think. If we don't want to document the return value of push, we should fix them all.

>     +                    (and class
>     +                         (progn (cl-pushnew class classes) t))))

Thinking about converting it to cl-pushnew is what made me look closer at this code in
the first place ;-)

Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Stefan Monnier
In reply to this post by Mattias Engdegård-2
>> I'd rather fix the code not to rely on the return value.
> I'm sure a lot more code relies on the return value of 'push'.

Probably, but I still think it's bad practice to use the return value of
an operation which is fundamentally a side-effect.


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Juanma Barranquero


On Tue, Dec 3, 2019 at 5:01 PM Stefan Monnier <[hidden email]> wrote:

> Probably, but I still think it's bad practice to use the return value of
> an operation which is fundamentally a side-effect.

So you've never written

(if (setq var value) ...)

?  ;-)
Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Eli Zaretskii
> From: Juanma Barranquero <[hidden email]>
> Date: Tue, 3 Dec 2019 17:06:16 +0100
> Cc: Mattias Engdegård <[hidden email]>,
>  Emacs developers <[hidden email]>
>
> So you've never written
>
> (if (setq var value) ...)
>
> ?  ;-)

The return value of setq _is_ documented.

Reply | Threaded
Open this post in threaded view
|

RE: master a6b5985: Avoid duplicated character classes in rx

Drew Adams
In reply to this post by Mattias Engdegård-2
> > I'd rather fix the code not to rely on the return value.
>
> I'm sure a lot more code relies on the return value of 'push'.
> It's also well-defined in Common Lisp. Why not just document it?

+1.

My guess is that this doc lack might have just
been an oversight.

Prior to Emacs 21 (or possibly 22), `push' was
defined only in `cl.el' (with no `cl-' prefix).

Its doc string said this:

  push is a Lisp macro in `cl.el'.
  (push X PLACE)

  (push X PLACE): insert X at the head of the list stored in PLACE.
  Analogous to (setf PLACE (cons X PLACE)), though more careful about
  evaluating each argument only once and in the right order.  PLACE may
  be a symbol, or any generalized variable allowed by `setf'.

IOW, even though that version of `push' was
supposed to be an Emacs emulation of Common
Lisp `push', there's no mention of the return
value.  There should have been.

`push' was moved outside of `cl.el' in Emacs 22
(or 21 possibly).  Its doc string was not fixed
to specify the return value.

`cl-pushnew' has the same doc problem: no spec
of the return value.

We should specify the return value for each of
these.  It is as important to `push' as it is
to `pop' (whose doc does specify the return
value).

Common Lisp specifies the return value:

http://clhs.lisp.se/Body/m_push.htm

Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Stefan Monnier
In reply to this post by Juanma Barranquero
>> Probably, but I still think it's bad practice to use the return value of
>> an operation which is fundamentally a side-effect.
> So you've never written
> (if (setq var value) ...)

Guilty as charged.  But every time I do so I feel a bit dirty (and not
just because I feel dirty every time I use `setq`).
And I have often regretted writing such code.

To my defense, `setq` is the only such exception I can think of.


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Juanma Barranquero
In reply to this post by Eli Zaretskii

On Tue, Dec 3, 2019 at 6:37 PM Eli Zaretskii <[hidden email]> wrote:

> The return value of setq _is_ documented.

Of course. But certainly, it is "the return value of an operation which is
fundamentally a side-effect", to quote Stefan. So every time he writes code
like that, some poor kitten is sacrificed to Cthulhu.

Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Juanma Barranquero
In reply to this post by Stefan Monnier

On Tue, Dec 3, 2019 at 6:40 PM Stefan Monnier <[hidden email]> wrote:

> Guilty as charged.

As we all are. Secretly or openly, all of us have done it occasionally.

> And I have often regretted writing such code.

;-)

> To my defense, `setq` is the only such exception I can think of.

I have sinned with `cl-incf' a few times, myself.

Reply | Threaded
Open this post in threaded view
|

RE: master a6b5985: Avoid duplicated character classes in rx

Drew Adams
In reply to this post by Eli Zaretskii
> > So you've never written
> > (if (setq var value) ...)
> > ?  ;-)
>
> The return value of setq _is_ documented.

Likewise `setf'.  Which is why the return
value of `push' is _indirectly_ documented.

That indirection already commits to support
of the return value (of `push').  We should
specify it explicitly.

As does Common Lisp (even though it too
refers to `setf' in the doc).  There's no
reason not to say clearly and directly what
`push' returns.

Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Stefan Monnier
In reply to this post by Juanma Barranquero
> So every time he writes code like that, some poor kitten is sacrificed
> to Cthulhu.

Now I feel even more guilty!


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Stefan Monnier
In reply to this post by Juanma Barranquero
>> To my defense, `setq` is the only such exception I can think of.
> I have sinned with `cl-incf' a few times, myself.

Now *this* is sick!
Your karma credit will not go back up until you atone for that,


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Juanma Barranquero

On Tue, Dec 3, 2019 at 7:36 PM Stefan Monnier <[hidden email]> wrote:

> Now *this* is sick!

I know.

> Your karma credit will not go back up until you atone for that,

I was for a long while a Perl / C++ programmer. My karma credit is chthonic.

I try to atone with Lisp and Ada, but life is too short.
Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Michael Welsh Duggan-3
In reply to this post by Stefan Monnier
Stefan Monnier <[hidden email]> writes:

>>> I'd rather fix the code not to rely on the return value.
>> I'm sure a lot more code relies on the return value of 'push'.
>
> Probably, but I still think it's bad practice to use the return value of
> an operation which is fundamentally a side-effect.

But isn't that standard practice?  From the elisp manual, for example:
(setq x (nreverse x))

--
Michael Welsh Duggan
([hidden email])

Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Stefan Monnier
>>>> I'd rather fix the code not to rely on the return value.
>>> I'm sure a lot more code relies on the return value of 'push'.
>> Probably, but I still think it's bad practice to use the return value of
>> an operation which is fundamentally a side-effect.
> But isn't that standard practice?  From the elisp manual, for example:
> (setq x (nreverse x))

I don't think `nreverse` is "fundamentally a side-effect": its return
value is of critical importance.

Similarly return values which correspond to side-information about how
the side-effect was done (e.g. status or error codes) aren't subject to
the above "rule".


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Richard Stallman
In reply to this post by Juanma Barranquero
[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > Probably, but I still think it's bad practice to use the return value of
  > > an operation which is fundamentally a side-effect.

  > So you've never written

  > (if (setq var value) ...)

It is clearer to write

  (setq var value)
  (if var ...)

--
Dr Richard Stallman
Founder, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)



Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Juanma Barranquero

On Wed, Dec 4, 2019 at 5:36 AM Richard Stallman <[hidden email]> wrote:

> It is clearer to write
>
>   (setq var value)
>   (if var ...)

Sure.

I think I've used (if (setq...) only in cases where it avoided an indentation level
(having to add `progn' or somesuch) in code already overstuffed.

Reply | Threaded
Open this post in threaded view
|

Re: master a6b5985: Avoid duplicated character classes in rx

Mattias Engdegård-2
In reply to this post by Stefan Monnier
Stefan Monnier <[hidden email]>:

> Probably, but I still think it's bad practice to use the return value of
> an operation which is fundamentally a side-effect.

Very well, it has now been wrapped in (progn ... t).

Juanma Barranquero <[hidden email]>:

> So you've never written
>
> (if (setq var value) ...)

Not so much, but I find it harder to avoid

  (while (setq v e) ...)

lacking tail-recursive loops.

> Thinking about converting it to cl-pushnew is what made me look closer at this code in
> the first place ;-)

I prefer not using cl in rx.el at all for messy bootstrap reasons. (cl-pushnew is a macro, but I'd rather not tempt fate.)


12