Skip to content

New PlainListCopy vs. AsPlist: unify? #4357

Open
@fingolfin

Description

In PR #4332, the GAP library function PlainListCopy (which has existed for many years but was undocumented) was fixed and enhanced, for use in PR #4331.

Today I came upon the GAP library operation AsPlist which seems to serve a very similar purpose, with one key difference: if the input is already a plain list, it just returns that, i.e., not a copy. That's also what the documentation of that function says (but note that it is not documented in the GAP reference manual, i.e., it is "undocumented" for purposes of whether we made any binding promises about it or not). Also, no package I have access to uses AsPlist

Note we already have a bunch of "pairs" like List/AsList, Set/AsSet; typically, the first function returns a mutable object, the As* version an immutable one. Based on that, it is also strange that AsPlist returns something mutable...

How about the following:

  • rename PlainListCopy to Plist (and/or have one be a synonym of the other)
  • change AsPlist to return an immutable copy instead
  • for efficiency reasons (dispatch where an argument is a list is slow), perhaps AsPlist should be a kernel operation or a
  • simple global function which only dispatches to an actual operation, say AsPlistOp)

On the other hand, it may be useful for efficiency reasons to have a function like the current AsPlist, i.e., which returns the original input if it was a plist all along. But looking at the GAP library, I don't really think it matters much if at all to the existing use cases, which mostly look like this: AsSSortedListList(AsPlist(xxx)). So yeah, if xxx is plain list, we save a bit, but typically the relevant methods already have a special case for IsPlistRep, e.g.

InstallOtherMethod(AsSSortedList,
    "for a plist",
    [IsList and IsPlistRep],
    AsSSortedListList );

InstallOtherMethod(AsSSortedList,
     "for a list",
     [ IsList ],
     l -> AsSSortedListList( AsPlist( l ) ) );

Thoughts? It would be good to resolve this before the GAP 4.12.0 release, so that we never announce PlainListCopy if we decide to rename it.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    kind: discussiondiscussions, questions, requests for comments, and so on

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions