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

Change the way we do Int64 conversion to/from GAP #357

Merged
merged 1 commit into from
Mar 6, 2020

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Mar 5, 2020

Previously, any Int64 too big to fit into a GAP immediate integer was put into
a T_JULIA wrapper when passed to GAP, and unwrapped later. At the same time,
julia_to_gap relied completely (and incorrectly) on the C kernel functions
julia_gap and gap_julia to take care of all Int64 values.

The result was that passing such an integer as argument to a GAP function did
not work, not even if one explicitly used julia_to_gap.

To fix this we teach the C kernel function gap_julia to convert "large"
Int64 values to GAP big integers; this ensures that the many adapter functions
we install for Int64 arguments work.

This means that we violate the principle of roundtrip type fidelity (Int64 -> GAP large
int -> MPtr), but that is a very minor concern.

For really efficient conversions of e.g. Array{Int64,1} to GAP plists, we need
dedicated conversion functions which directly copy over bits. These can be
added transparently in the future.

Finally, we add some minor optimization to julia_to_gap for UInt64, Int128, UInt128

@codecov
Copy link

codecov bot commented Mar 5, 2020

Codecov Report

Merging #357 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
- Coverage   77.77%   77.74%   -0.04%     
==========================================
  Files          58       58              
  Lines        3995     3994       -1     
==========================================
- Hits         3107     3105       -2     
- Misses        888      889       +1     

@fingolfin
Copy link
Member Author

@ThomasBreuer I have now completely rewritten this. Tests pass

@fingolfin fingolfin force-pushed the mh/Int64 branch 2 times, most recently from c149b1b to 9358a53 Compare March 5, 2020 16:05
Previously, any Int64 too big to fit into a GAP immediate integer was put into
a T_JULIA wrapper when passed to GAP, and unwrapped later. At the same time,
`julia_to_gap` relied completely (and incorrectly) on the C kernel functions
`julia_gap` and `gap_julia` to take care of *all* Int64 values.

The result was that passing such an integer as argument to a GAP function did
not work, not even if one explicitly used `julia_to_gap`.

To fix this we teach the C kernel function `gap_julia` to convert "large"
Int64 values to GAP big integers; this ensures that the many adapter functions
we install for Int64 arguments work.

This means that we violate the principle of roundtrip type fidelity (Int64 -> GAP large
int -> MPtr), but that is a very minor concern.

For really efficient conversions of e.g. Array{Int64,1} to GAP plists, we need
dedicated conversion functions which directly copy over bits. These can be
added transparently in the future.

Finally, we add some minor optimization to julia_to_gap for UInt64, Int128, UInt128
Copy link
Member

@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.

Looks good. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants