GNU bug report logs - #38000
27.0.50; [PATCH] WIP on using gnus info accessor macros

Previous Next

Package: emacs;

Reported by: Eric Abrahamsen <eric <at> ericabrahamsen.net>

Date: Thu, 31 Oct 2019 00:00:02 UTC

Severity: normal

Tags: patch

Found in version 27.0.50

Fixed in version 27.1

Done: Eric Abrahamsen <eric <at> ericabrahamsen.net>

Bug is archived. No further changes may be made.

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

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

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#38000; Package emacs. (Thu, 31 Oct 2019 00:00:03 GMT) Full text and rfc822 format available.

Acknowledgement sent to Eric Abrahamsen <eric <at> ericabrahamsen.net>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Thu, 31 Oct 2019 00:00:03 GMT) Full text and rfc822 format available.

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

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: bug-gnu-emacs <at> gnu.org
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: 27.0.50; [PATCH] WIP on using gnus info accessor macros
Date: Wed, 30 Oct 2019 16:58:52 -0700
[Message part 1 (text/plain, inline)]
The attached patch is a work-in-progress for making sure that all of
Gnus' code only accesses group infos via the relevant macros --
essentially hiding the fact that group infos are implemented as lists,
in anticipation of eventually being able to re-implement them as structs
at some point in the future.

Essentially, "(nth 3 info)" becomes "(gnus-info-marks info)", and so
one.

It also replaces uses of "gnus-info-set-*" with "(setf (gnus-info-* ".
The setter macros aren't deprecated, though.

My main question is about the use of the EXTEND argument to
`gnus-info-set-marks', `gnus-info-set-method', and
`gnus-info-set-params'. In the current patch, I have left the setter
intact wherever the EXTEND argument is passed, until I can figure this
out.

If I could understand why _some_ of the infos are expected to be short a
few elements (newly-created groups?), I would rather provide a
`make-gnus-info' function (again, in anticipation of a struct
constructor), which could be called with the elements of an info (either
as a list or spread) and return an info filled-out with nils where
necessary. Then that could be used wherever we expect a short info.

There's also the weird spot in gnus-sum.el:6183 where we're chopping off
nil elements from the end of an info list, I don't know why.

One last bit I'm uncertain about is in `nnvirtual-request-update-info',
where I've replaced this:

      (setcar (cddr info) nnvirtual-mapping-reads)
      (if (nthcdr 3 info)
	  (setcar (nthcdr 3 info) nnvirtual-mapping-marks)
	(when nnvirtual-mapping-marks
	  (setcdr (nthcdr 2 info) (list nnvirtual-mapping-marks))))

with this:

      (setf (gnus-info-read info) nnvirtual-mapping-reads)
      (when nnvirtual-mapping-marks
	(setf (gnus-info-marks info) nnvirtual-mapping-marks))

It seems to work fine, but to be honest I don't really understand the
original logic.

Eric

[0001-WIP-on-using-gnus-info-accessors.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#38000; Package emacs. (Thu, 31 Oct 2019 12:16:01 GMT) Full text and rfc822 format available.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: 27.0.50; [PATCH] WIP on using gnus info accessor macros
Date: Thu, 31 Oct 2019 13:15:31 +0100
Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:

> The attached patch is a work-in-progress for making sure that all of
> Gnus' code only accesses group infos via the relevant macros --
> essentially hiding the fact that group infos are implemented as lists,
> in anticipation of eventually being able to re-implement them as structs
> at some point in the future.
>
> Essentially, "(nth 3 info)" becomes "(gnus-info-marks info)", and so
> one.
>
> It also replaces uses of "gnus-info-set-*" with "(setf (gnus-info-* ".
> The setter macros aren't deprecated, though.

Sounds good, but you can do this with the cl-defstruct list thing
already without changing what the data is.

(cl-defstruct (gnus-info
               (:constructor make-gnus-info)
               (:copier nil)
               (:type list))
 (group ...))

Actually changing the data format itself I'm not very enthusiastic
about.  There's tons of out-of-tree code that would break.

> If I could understand why _some_ of the infos are expected to be short a
> few elements (newly-created groups?), I would rather provide a
> `make-gnus-info' function (again, in anticipation of a struct
> constructor), which could be called with the elements of an info (either
> as a list or spread) and return an info filled-out with nils where
> necessary. Then that could be used wherever we expect a short info.

I think it's just because the data is written to .newsrc.eld as is, and
writing a bunch of nils there would make the file a lot bigger.
Remember, people may have thousands of groups.

> One last bit I'm uncertain about is in `nnvirtual-request-update-info',
> where I've replaced this:
>
>       (setcar (cddr info) nnvirtual-mapping-reads)
>       (if (nthcdr 3 info)
> 	  (setcar (nthcdr 3 info) nnvirtual-mapping-marks)
> 	(when nnvirtual-mapping-marks
> 	  (setcdr (nthcdr 2 info) (list nnvirtual-mapping-marks))))
>
> with this:
>
>       (setf (gnus-info-read info) nnvirtual-mapping-reads)
>       (when nnvirtual-mapping-marks
> 	(setf (gnus-info-marks info) nnvirtual-mapping-marks))
>
> It seems to work fine, but to be honest I don't really understand the
> original logic.

I don't understand it either, but it's setting both the second and the
fourth slot, isn't it?

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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#38000; Package emacs. (Thu, 31 Oct 2019 19:18:02 GMT) Full text and rfc822 format available.

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

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 38000 <at> debbugs.gnu.org
Subject: Re: bug#38000: 27.0.50; [PATCH] WIP on using gnus info accessor macros
Date: Thu, 31 Oct 2019 12:17:28 -0700
Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:
>
>> The attached patch is a work-in-progress for making sure that all of
>> Gnus' code only accesses group infos via the relevant macros --
>> essentially hiding the fact that group infos are implemented as lists,
>> in anticipation of eventually being able to re-implement them as structs
>> at some point in the future.
>>
>> Essentially, "(nth 3 info)" becomes "(gnus-info-marks info)", and so
>> one.
>>
>> It also replaces uses of "gnus-info-set-*" with "(setf (gnus-info-* ".
>> The setter macros aren't deprecated, though.
>
> Sounds good, but you can do this with the cl-defstruct list thing
> already without changing what the data is.
>
> (cl-defstruct (gnus-info
>                (:constructor make-gnus-info)
>                (:copier nil)
>                (:type list))
>  (group ...))
>
> Actually changing the data format itself I'm not very enthusiastic
> about.  There's tons of out-of-tree code that would break.

Huh, I didn't realize that list-based structs didn't put a tag in the
`car' position unless you pass the :named keyword.

We could switch to list-based structs without breaking out-of-tree code,
but I don't see much point: you can't use them for generic function
dispatch, even when they're :named, nor do you get a NAME-p predicate
for free. That doesn't leave much point.

I think we can just stick with hiding the implementation for now,
encourage other developers not to use `nth', and maybe someday in the
future switch to record-based structs.

>> If I could understand why _some_ of the infos are expected to be short a
>> few elements (newly-created groups?), I would rather provide a
>> `make-gnus-info' function (again, in anticipation of a struct
>> constructor), which could be called with the elements of an info (either
>> as a list or spread) and return an info filled-out with nils where
>> necessary. Then that could be used wherever we expect a short info.
>
> I think it's just because the data is written to .newsrc.eld as is, and
> writing a bunch of nils there would make the file a lot bigger.
> Remember, people may have thousands of groups.

Okay, maybe all that stuff should just stay as it is, then. Alternately,
I could do something fancy with gv-setters for `gnus-info-marks' et al,
so that setting them via `setf' would first ensure their length.

Alternately, we could pad out each info as it is read from .newsrc.eld,
and vacuum out nils again before it is written. Then the rest of the
code could just forget about it.

>> One last bit I'm uncertain about is in `nnvirtual-request-update-info',
>> where I've replaced this:
>>
>>       (setcar (cddr info) nnvirtual-mapping-reads)
>>       (if (nthcdr 3 info)
>> 	  (setcar (nthcdr 3 info) nnvirtual-mapping-marks)
>> 	(when nnvirtual-mapping-marks
>> 	  (setcdr (nthcdr 2 info) (list nnvirtual-mapping-marks))))
>>
>> with this:
>>
>>       (setf (gnus-info-read info) nnvirtual-mapping-reads)
>>       (when nnvirtual-mapping-marks
>> 	(setf (gnus-info-marks info) nnvirtual-mapping-marks))
>>
>> It seems to work fine, but to be honest I don't really understand the
>> original logic.
>
> I don't understand it either, but it's setting both the second and the
> fourth slot, isn't it?

I'm pretty sure the first line (setting the read slot) is correct.

Then the if statement... "if the info has marks or method or params, set
the marks to nnvirtual-mapping-marks, regardless of whether
nnvirtual-mapping-marks is nil or not, and overwriting whatever the
marks were before. Otherwise, if the info has no marks and
nnvirtual-mapping-marks is non-nil, set the info marks to
nnvirtual-mapping-marks."

Nope, still don't get it. It seems to me the only real question is
whether or not nnvirtual-mapping-marks should override whatever marks
were already there.

Eric




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#38000; Package emacs. (Fri, 01 Nov 2019 13:45:02 GMT) Full text and rfc822 format available.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: 38000 <at> debbugs.gnu.org
Subject: Re: bug#38000: 27.0.50; [PATCH] WIP on using gnus info accessor macros
Date: Fri, 01 Nov 2019 14:44:55 +0100
Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:

> Huh, I didn't realize that list-based structs didn't put a tag in the
> `car' position unless you pass the :named keyword.
>
> We could switch to list-based structs without breaking out-of-tree code,
> but I don't see much point: you can't use them for generic function
> dispatch, even when they're :named, nor do you get a NAME-p predicate
> for free. That doesn't leave much point.

It means that you don't have to write the accessor macros by hand.  :-)

> Nope, still don't get it. It seems to me the only real question is
> whether or not nnvirtual-mapping-marks should override whatever marks
> were already there.

I don't get it either, I'm afraid.

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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#38000; Package emacs. (Fri, 01 Nov 2019 18:08:02 GMT) Full text and rfc822 format available.

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

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 38000 <at> debbugs.gnu.org
Subject: Re: bug#38000: 27.0.50; [PATCH] WIP on using gnus info accessor macros
Date: Fri, 01 Nov 2019 11:07:03 -0700
Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:
>
>> Huh, I didn't realize that list-based structs didn't put a tag in the
>> `car' position unless you pass the :named keyword.
>>
>> We could switch to list-based structs without breaking out-of-tree code,
>> but I don't see much point: you can't use them for generic function
>> dispatch, even when they're :named, nor do you get a NAME-p predicate
>> for free. That doesn't leave much point.
>
> It means that you don't have to write the accessor macros by hand.  :-)

But you already did! :)

Okay, let's use them anyway. And I'd like to move the "extend" and
"vacuum" procedures to the reading and writing of the newsrc.eld file,
so the rest of the code can forget about it. I'll leave the setter
macros in place, and continue to accept (and ignore) an EXTEND argument,
so other people's code continues to work.

>> Nope, still don't get it. It seems to me the only real question is
>> whether or not nnvirtual-mapping-marks should override whatever marks
>> were already there.
>
> I don't get it either, I'm afraid.

I'll look at it some more, and see if I can come up with some tests.

Eric




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#38000; Package emacs. (Fri, 01 Nov 2019 22:53:01 GMT) Full text and rfc822 format available.

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

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 38000 <at> debbugs.gnu.org
Subject: Re: bug#38000: 27.0.50; [PATCH] WIP on using gnus info accessor macros
Date: Fri, 01 Nov 2019 15:52:44 -0700
[Message part 1 (text/plain, inline)]
Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:

> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
>
>> Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:
>>
>>> Huh, I didn't realize that list-based structs didn't put a tag in the
>>> `car' position unless you pass the :named keyword.
>>>
>>> We could switch to list-based structs without breaking out-of-tree code,
>>> but I don't see much point: you can't use them for generic function
>>> dispatch, even when they're :named, nor do you get a NAME-p predicate
>>> for free. That doesn't leave much point.
>>
>> It means that you don't have to write the accessor macros by hand.  :-)
>
> But you already did! :)
>
> Okay, let's use them anyway. And I'd like to move the "extend" and
> "vacuum" procedures to the reading and writing of the newsrc.eld file,
> so the rest of the code can forget about it. I'll leave the setter
> macros in place, and continue to accept (and ignore) an EXTEND argument,
> so other people's code continues to work.

Okay, here's what the patch looks like now.

1. gnus-info is defined with defstruct, and the constructor arglist
looks like: (group rank read &optional marks method params), meaning
that if you have an info list that may or may not be fully extended, you
can run it through the constructor and all the missing elements will be
set to their default (nil).

2. This is done when reading the newsrc.eld file (and creating new
groups), and all the other uses of the 'extend argument have been
removed.`

3. The bit that chops off nils from the end of infos has been moved from
`gnus-update-marks' to `gnus-gnus-to-quick-newsrc-format', so it only
happens right at the last minute, when saving.

4. I've changed `gnus-set-info' to update each element of an info
individually, rather than just replacing the whole thing. This looks
stupid, but preserves object reference, meaning we no longer need to mess
with the `gnus-newsrc-alist' in `gnus-group-set-info', which was causing
trouble for people. It's going to be a bit slower, but I don't think this is
super performance-crucial.

I think that's it... This still needs some testing, but I'm optimistic.

[0001-WIP-on-using-gnus-info-accessors.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#38000; Package emacs. (Sat, 02 Nov 2019 14:44:02 GMT) Full text and rfc822 format available.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: 38000 <at> debbugs.gnu.org
Subject: Re: bug#38000: 27.0.50; [PATCH] WIP on using gnus info accessor macros
Date: Sat, 02 Nov 2019 15:43:45 +0100
Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:

> Okay, here's what the patch looks like now.

[...]

> I think that's it... This still needs some testing, but I'm optimistic.

It all sounds good to me.  I've just skimmed the patch (which is very
big, but mostly with "mechanical" substitutions), and it looks fine to
me.  But, yes, thorough testing would be nice.

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




bug marked as fixed in version 27.1, send any further explanations to 38000 <at> debbugs.gnu.org and Eric Abrahamsen <eric <at> ericabrahamsen.net> Request was from Eric Abrahamsen <eric <at> ericabrahamsen.net> to control <at> debbugs.gnu.org. (Tue, 17 Dec 2019 18:42:02 GMT) Full text and rfc822 format available.

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Wed, 15 Jan 2020 12:24:04 GMT) Full text and rfc822 format available.

This bug report was last modified 4 years and 74 days ago.

Previous Next


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