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.

Sign up for GitHub

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

Jump to bottom

Add GAP_NewRange, GAP_NewObjIntFromInt, GAP_ValueInt to the libgap API #4621

Merged
merged 2 commits into from
Aug 4, 2021
Merged

Add GAP_NewRange, GAP_NewObjIntFromInt, GAP_ValueInt to the libgap API #4621

merged 2 commits into from
Aug 4, 2021

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Jul 27, 2021 •
edited
Loading

This is cleanup, but it will also be used in the GAP-Julia interface. Might also be useful for other libgap users.

TODO: This needs some more work, in particular the libgap API functions needs to be documented.

@fingolfin fingolfin added topic: kernel topic: libgap things related to libgap topic: julia Julia GC integration and related matters release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Jul 27, 2021
@fingolfin fingolfin mentioned this pull request Jul 27, 2021
Closed
ChrisJefferson
ChrisJefferson reviewed Jul 27, 2021
View reviewed changes
src/libgap-api.c Outdated
{
if (!fitsInIntObj(len)) return 0;
if (!fitsInIntObj(low)) return 0;
if (!fitsInIntObj(inc)) return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you just want to catch inc==0 here too? I don't think it's allowed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done

@fingolfin fingolfin marked this pull request as ready for review July 29, 2021 10:20
@fingolfin fingolfin changed the title Add GAP_NewRange to the libgap API Add GAP_NewRange, GAP_NewObjIntFromInt, GAP_ValueInt to the libgap API Jul 29, 2021
@fingolfin fingolfin linked an issue Jul 29, 2021 that may be closed by this pull request
libgap: add GAP_ValueSmallInt or something like it #4208
Closed
@fingolfin
Copy link
Member Author

I've now enhanced this a bit more to resolves issue #4208 by @embray who also may have comments on this PR.

ThomasBreuer
ThomasBreuer approved these changes Jul 29, 2021
View reviewed changes
Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for curiosity: Why fitsInIntObj and not fitsInObjInt?
(ObjInt is used in most situations in the src files.)

(And there are two instances of "an GAP integer object" in comments.)

@fingolfin
Copy link
Member Author

@ThomasBreuer "intobj" refers to immediate ints (cf. IS_INTOBJ, INT_INTOBJ, and many more), while objint refers to any kind of GAP integer, i.e., "an Obj representing an integer"; the latter is relatively rarely used, though, mostly in ObjInt_Int and Int_ObjInt (and there many variants; but the variants all derive from those two; as does MakeObjInt).

It is rather confusing.

@ThomasBreuer ThomasBreuer merged commit 1821095 into gap-system:master Aug 4, 2021
@fingolfin fingolfin deleted the mh/NewRange branch August 4, 2021 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisJefferson ChrisJefferson ChrisJefferson left review comments

@ThomasBreuer ThomasBreuer ThomasBreuer approved these changes

Assignees
No one assigned
Labels
release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: julia Julia GC integration and related matters topic: kernel topic: libgap things related to libgap
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

libgap: add GAP_ValueSmallInt or something like it
3 participants
@fingolfin @ChrisJefferson @ThomasBreuer