bug#38000: 27.0.50; [PATCH] WIP on using gnus info accessor macros

[Eric Abrahamsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eric Abrahamsen <eric@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