Rewrite and officially document PlainListCopy

[ChrisJefferson]

After looking at uses of PlainListCopy, I decided the internal kernel function PLAIN_LIST_COPY does everything it is supposed to do, and the best idea was to delegate to it.

While PlainListCopy could (by it's docs) sometimes copy and sometimes not, none of the uses we relying on the behaviour of not copying, or changing the original.

This mainly involved removing the now unused PlainListCopyOp.

This PR is required for #4331

[ChrisJefferson]

Suggested release notes:

Add function PlainListCopy which takes a list and returns a copy in "plain list" representation. (Note: an undocumented function with this name existed in GAP for a long time, but it had bugs and did not work as intended in all cases.)

[wilfwilson]

Thanks for separating this into a separate PR. I've got a few questions/suggestions.

[wilfwilson]

I think it would be good to document that the returned list is guaranteed to be an actual copy (i.e. it's equal but not IsIdenticalObj)

[wilfwilson]

Thanks for making those changes. Here's a concrete suggestion for how you could implement the remaining comment from my first review.

[wilfwilson]

Suggested change

	## This function returns a list equal to its argument, in a plain list
	## representation.
	## This function returns a new list that is equal to its argument
	## and is in the plain list representation.

[fingolfin]

This exists and was a typo for ConstantTimeAccessList

[fingolfin]

That said, I don't think this sentence was helpful, so I am fine with removing it :-)

[ChrisJefferson]

I'm going to go though all the callers of PlainListCopy.

Firstly, there a number of lines of the form x := PlainListCopy(x) in listcoef.gi. These lines include other attempts to convert between representations. As we are assigning the return, it doesn't make any difference if the original was returned, or copied.

The function PLAIN_SL tries to convert a sparse list in place to a plain list. It does: CLONE_OBJ(sl, PlainListCopy(sl));, so once again it doesn't matter if PlainListCopy returns the original, or a copy. I checked this function does work correctly if PlainListCopy does, or doesn't return a copy. This function is very weird looking, but no need to change it today.

Finally, tuples.gi calls PlainListCopy on a newly created list. I imagine here it is because List can "be clever" and return a weird type of list.

[fingolfin]

I mildly disagree that x := PlainListCopy(x) implies it doesn't matter whether we modified the original x in place or not: sure, we don't reference it anymore in x afterwards, but it still might be referenced somewhere else, and so the modification still be visible there... However, all calls in listcoef.gi are preceded by a check of the form if q <> ConvertToVectorRepNC(vec,q) so we tried inplace conversion of the vector and it failed, which should mean that the vector is not compressed, and so we are good. (Extra crazy weirdness could occurs if vec is compressed but cannot be stored compressed over GF(q), but that shouldn't be possible here, and if it is, I frankly don't care, then this code is bizarre and broken). So in the end, I agree with you, this part is fine.

The whole "sparse list" code is undocumented, unused, not loaded, and AFAIK incomplete. We may as well delete it (opened PR #4336).

I really don't understand why it is used in tuples.gi (wish there was a comment explaining it!) but I am willing to risk it ;-).

Finally, no packages seem to be using those. Good!

[fingolfin]

Looks good to me but perhaps review the couple comments by Wilf and myself, and decide if you want to do anything about any of them

[fingolfin]

I took the liberty and adjusted the suggested release notes. Feel free to revise further.

[fingolfin]

Should we say here that it is a kernel function, in case people wonder where to find it?

[fingolfin]

(I am also fine with merging as-is, just wondering)

[ChrisJefferson]

I've now moved the documentation inline in the doc XML, because it was weird here -- but maybe that's a bad idea too. We could start scanning C code for GAPDoc?

[fingolfin]

In doc/ref/makedocreldata.g we already list src/sysfiles.c and could add more. I would in fact suggest that we change that file to not hardcode the list of files to scan, but rather to scan all files src/*.{c,h}, lib/*.{gd,gi}, etc., but of course that goes way beyond the scope of this PR ;-)

[wilfwilson]

Sorry I must have touched my trackpad really awkwardly and closed the PR and posted this comment before I was finished!! Whoops.

The CI failures...

In testbugfix - CONFIGFLAGS="--enable-memory-checking" - ubuntu-18.04 there was another instance of this happening (re-running the tests, which I've accidentally triggered by closing and reopening, should make this go away):

########> Diff in /home/runner/work/gap/gap/tst/testbugfix/2006-08-28-t00\
151.tst:14
# Input is:
if time1 >= 2*time2 then
Print("Bad timings for bugfix 2006/08/28 (FL): ", time1, " >= 2*", time2, "\n"\
); 
fi; # time1 and time2 should be about the same
# Expected output:
# But found:
Bad timings for bugfix 2006/08/28 (FL): 19430 >= 2*4959
########

and in docomp testinstall - HPCGAP=yes ABI=64 - ubuntu-18.04 there is:

testing: /home/runner/work/gap/gap/tst/testinstall/list.tst
########> Diff in /home/runner/work/gap/gap/tst/testinstall/list.tst:
342
# Input is:
PlainListCopy(Group((1,2)));
# Expected output:
Error, PlainListCopy: <list> must be a small list (not a component o\
bject)
# But found:
Error, PlainListCopy: <list> must be a small list (not an atomic com\
ponent obj\
ect)
########

Which seems a bit harder to fix, if HPCGAP gives different output. (Maybe change the test to use something other than a group as its example?).

[fingolfin]

Still looks good to me, please merge at will