-
Notifications
You must be signed in to change notification settings - Fork 178
List extensions: allow arbitrary objects as list "indices" #28
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
Conversation
|
There is the obvious complaint -- no new tests of this new indexing method! |
|
But, I don't have any other comments -- this all looks correct. Also having recently been in the parser, I think supporting |
|
👍 so let's just add some tests and merge this! |
|
@fingolfin this PR can't be merged automatically, and I think PR #83 by @stevelinton depends on your PR. Will you be able to merge it manually, please? I suggest to add tests later to test new syntax in all related PRs. |
b724b26 to
b73b0e6
Compare
|
I rebased the series so that it should merge cleanly again. I am now working on some tests. Regarding documentation: I'll need to look at the manual to figure out how difficult that would be. Oh, and of course the release news would then also have to reflect this, I guess.. ? |
… bit of old commented out code relating to the equivalent change for access some time ago.
Moreover, - use of IS_POS_INTOBJ in multiple places for readability and efficiency - enforce consistent behavior for ELM/ASS/ISB/UNB, and also between interpreted and compiled code: positive integers as before, while anything else (including non-positive integers) is sent through method dispatch - some code cleanup (indention, uniform whitespace in comments, etc.)
b73b0e6 to
8c7127d
Compare
|
Thanks, @fingolfin. Test passed - may be merged now? Regarding release news - I will create a stab for the "Changes" manual chapter on GAP 4.8, then we may put an overview of all major changes there. Release announcement may be based on that chapter. |
|
No, I do not think we should merge syntax extensions before they are documented and tested! There are already far too many things littered throughout the GAP codebase which somebody added, probably with the plan to document them later, but this then never happened, and they clutter up things. |
|
Superceded by #192 |
The changes in this pull request make it possible to implement methods for [], []:=, etc. which take arbitrary objects as "index", not just positive integers. At the same time, it unifies some code and fixes a few places which should have only accepted positive integers, but actually accepted arbitrary integers.
The original motivation for this patch was to make it possible to implement matrix methods which take a pair of integers, like so:
to se the entry in row i, column j of the matrix a to 2.
Internally, this would invokes the []:= method with the parameters "a" and "[i,j]", and the method can then set the matrix entry. E.g. for IsMatrixObj objects, it could invoke SetMatElm.
There are other applications, however, e.g. for nicer interfaces to dictionaries:
The biggest missing thing in this PR is a documentation update, and some review by somebody other than Steve and me. If the changes look sane, we could merge them now, and coument the changes later.
BTW, for the matrix application, it would be even nicer if we had a language extensions that would allow directly writing
i.e. without the double brackets. But this is apparently harder to achieve (?), and could still be done later, and I think the changes presented here are useful for other applications (such as dictionaries / maps).