GNU bug report logs - #44471
[PATCH] Simplify text-quoting-style

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 44471 in the body.
You can then email your comments to 44471 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Simplify text-quoting-style
Date: Thu, 5 Nov 2020 08:18:21 -0800

[Message part 1 (text/plain, inline)]

I found an opportunity to simplify the code for text-quoting-style.  See
the attached patch.

The patch also improves the name of the defun `get-quoting-style' by
changing it to `text-quoting-style'.  (This new name matches the old C
function name and the existing variable name.)

Any comments?

[0001-Simplify-text-quoting-style.patch (text/x-diff, attachment)]

Message #8 received at 44471 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 44471 <at> debbugs.gnu.org
Subject: Re: bug#44471: [PATCH] Simplify text-quoting-style
Date: Mon, 09 Nov 2020 16:56:24 +0100

Stefan Kangas <stefan <at> marxist.se> writes:

> I found an opportunity to simplify the code for text-quoting-style.  See
> the attached patch.
>
> The patch also improves the name of the defun `get-quoting-style' by
> changing it to `text-quoting-style'.  (This new name matches the old C
> function name and the existing variable name.)
>
> Any comments?

Makes sense to me.  And since get-quoting-style is new in Emacs 28 (I
think?), no deprecation should be necessary.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #11 received at 44471 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 44471 <at> debbugs.gnu.org
Subject: Re: bug#44471: [PATCH] Simplify text-quoting-style
Date: Mon, 9 Nov 2020 12:13:23 -0800

tags 44471 fixed
close 44471 28.1
thanks

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Stefan Kangas <stefan <at> marxist.se> writes:
>
>> I found an opportunity to simplify the code for text-quoting-style.  See
>> the attached patch.
>>
>> The patch also improves the name of the defun `get-quoting-style' by
>> changing it to `text-quoting-style'.  (This new name matches the old C
>> function name and the existing variable name.)
>>
>> Any comments?
>
> Makes sense to me.

Thanks.  There have been no other comments, so I've pushed this to
master as commit 95c04675ab.

> And since get-quoting-style is new in Emacs 28 (I think?), no
> deprecation should be necessary.

Yes, it is not even a month old (and added by yours truly).

Message #18 received at 44471 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 44471 <at> debbugs.gnu.org
Subject: Re: bug#44471: [PATCH] Simplify text-quoting-style
Date: Tue, 10 Nov 2020 15:30:09 +0100

Stefan Kangas <stefan <at> marxist.se> writes:

> Yes, it is not even a month old (and added by yours truly).

Right.  :-)  Perhaps this function should have a NEWS entry?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #21 received at 44471 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: larsi <at> gnus.org, 44471 <at> debbugs.gnu.org
Subject: Re: bug#44471: [PATCH] Simplify text-quoting-style
Date: Tue, 10 Nov 2020 18:12:31 +0200

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Mon, 9 Nov 2020 12:13:23 -0800
> Cc: 44471 <at> debbugs.gnu.org
> 
> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> 
> > Stefan Kangas <stefan <at> marxist.se> writes:
> >
> >> I found an opportunity to simplify the code for text-quoting-style.  See
> >> the attached patch.
> >>
> >> The patch also improves the name of the defun `get-quoting-style' by
> >> changing it to `text-quoting-style'.  (This new name matches the old C
> >> function name and the existing variable name.)
> >>
> >> Any comments?
> >
> > Makes sense to me.
> 
> Thanks.  There have been no other comments, so I've pushed this to
> master as commit 95c04675ab.

Please in the future allow more than just a couple of days for people
to comment on non-trivial patches.

As it happens, I do have a few issues with the change:

  . it slows down code of two very popular functions, because we now
    use EQ instead of a C-level equality operator;
  . it introduces a function that has the same name as a variable,
    which adds a bit to confusion, and also defeats our method of
    reporting in what version was the function/variable introduced
    (try "C-h f"); and
  . I don't think I see the simplification that justifies these
    (admittedly quite minor) downsides

Thanks.

Message #24 received at 44471 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, 44471 <at> debbugs.gnu.org
Subject: Re: bug#44471: [PATCH] Simplify text-quoting-style
Date: Wed, 11 Nov 2020 14:12:24 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

> Please in the future allow more than just a couple of days for people
> to comment on non-trivial patches.

OK.

> As it happens, I do have a few issues with the change:
>
>   . it slows down code of two very popular functions, because we now
>     use EQ instead of a C-level equality operator;

If I understand correctly, EQ(x, y) ends up in a C-level equality
operator and a typecast.  Is that correct?  If yes, is it the typecast
that you think will cause a slowdown, or is it something else?

I naively thought that this would not make much of a difference, and
that, to the extent that it did, the relevant functions are hardly
called often enough to matter in the end.  But I assume you know more
than me here, so please tell me where I'm wrong.

>   . it introduces a function that has the same name as a variable,
>     which adds a bit to confusion, and also defeats our method of
>     reporting in what version was the function/variable introduced
>     (try "C-h f"); and

This is not without precedent, see e.g. `user-full-name'.  But if we
want to keep my patch we could of course just use the previous name
`get-text-style' instead.

>   . I don't think I see the simplification that justifies these
>     (admittedly quite minor) downsides

I agree it is minor, but I see one function less, one less enum, and
overall fewer lines of code.

I'm happy to revert it if the change is not wanted.

Message #27 received at 44471 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: larsi <at> gnus.org, 44471 <at> debbugs.gnu.org
Subject: Re: bug#44471: [PATCH] Simplify text-quoting-style
Date: Wed, 11 Nov 2020 21:24:46 +0200

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Wed, 11 Nov 2020 14:12:24 -0500
> Cc: larsi <at> gnus.org, 44471 <at> debbugs.gnu.org
> 
> >   . it slows down code of two very popular functions, because we now
> >     use EQ instead of a C-level equality operator;
> 
> If I understand correctly, EQ(x, y) ends up in a C-level equality
> operator and a typecast.  Is that correct?

No.  EQ either expands to a call to lisp_h_EQ, or calls lisp_h_EQ,
which does:

  #define lisp_h_EQ(x, y) (XLI (x) == XLI (y))

Depending on how Lisp_Object is represented, this could be a typecast
or an access to a struct member.

> If yes, is it the typecast
> that you think will cause a slowdown, or is it something else?

the function call and the struct access.

> I naively thought that this would not make much of a difference, and
> that, to the extent that it did, the relevant functions are hardly
> called often enough to matter in the end.

The difference is very minor, but the point is that it's a (minor)
disadvantage.

> >   . it introduces a function that has the same name as a variable,
> >     which adds a bit to confusion, and also defeats our method of
> >     reporting in what version was the function/variable introduced
> >     (try "C-h f"); and
> 
> This is not without precedent, see e.g. `user-full-name'.  But if we
> want to keep my patch we could of course just use the previous name
> `get-text-style' instead.
> 
> >   . I don't think I see the simplification that justifies these
> >     (admittedly quite minor) downsides
> 
> I agree it is minor, but I see one function less, one less enum, and
> overall fewer lines of code.
> 
> I'm happy to revert it if the change is not wanted.

I don't have strong opinions, so I will leave it to you and others.

My point, though, is that minor cleanups should preferably not have
adverse effects, otherwise their benefit is questionable.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.