Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New PlainListCopy vs. AsPlist: unify? #4357

Open
fingolfin opened this issue Mar 29, 2021 · 3 comments
Open

New PlainListCopy vs. AsPlist: unify? #4357

fingolfin opened this issue Mar 29, 2021 · 3 comments
Labels
kind: discussion discussions, questions, requests for comments, and so on

Comments

@fingolfin
Copy link
Member

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.

@fingolfin fingolfin added the kind: discussion discussions, questions, requests for comments, and so on label Mar 29, 2021
@ChrisJefferson
Copy link
Contributor

ChrisJefferson commented Mar 29, 2021

I would support moving to renaming PlainListCopy to AsPlist (and throwing away the current AsPlist implementation). I don't really like functions that might, or might not, return a copy, I'd prefer it people really don't want a copy they explicitly check (as with the AsSSortedList example you give).

@fingolfin
Copy link
Member Author

But I was not suggesting to rename PlainListCopy to AsPlist, as I think AsPlist is also badly named -- other AsFOO functions return something immutable.

I guess I am suggesting several separate things:

  1. to merge PlainListCopy to AsPlist into one function.
  2. to not keep the name AsPlist as it is inconsistent.
  3. to actually call the new function Plist.

But thinking about point 3 again, that mostly makes sense if we also want to have an AsPlist version returns an immutable plist... otherwise, perhaps PlainListCopy or PlainList is better? I dunno -- but what I care about more are points 1 and 2 anyway. So e.g. we could retain the name PlainListCopy and just replace AsPlist by it...

@ChrisJefferson
Copy link
Contributor

Sorry, I should have read your message more carefully.

I think we should aim for consistency, so having both Plist, and AsPlist, seems sensible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: discussion discussions, questions, requests for comments, and so on
Projects
None yet
Development

No branches or pull requests

2 participants