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

gap: switch more code to offical libgap APIs #35761

Merged
merged 1 commit into from
Jul 1, 2023
Merged

gap: switch more code to offical libgap APIs #35761

merged 1 commit into from
Jul 1, 2023

Conversation

fingolfin
Copy link
Contributor

No description provided.

dcoudert
dcoudert suggested changes Jun 13, 2023
View reviewed changes
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

Unfortunately, I'm not able to help more than that.

@fingolfin

This comment was marked as outdated.

Sign in to view
@fingolfin
Copy link
Contributor Author

Seems there is a segfault now, due to a bad longjmp. Huh.

Maybe this is a GC issue: I just noticed that GapElement also calls (de)reference_obj which now is missing for the newly created list in make_gap_list, so that could be a problem (it still seems unlikely, but everything else in this PR should be basically equivalent replacements).,

So I'll remove that from this PR for now, I can still submit it again (and debug it) in a future PR. Of course if that doesn't resolve the issue, then some more digging is needed...

@fingolfin fingolfin force-pushed the mh/more-libgap branch 2 times, most recently from 58611ca to d3d9899 Compare June 19, 2023 06:53
@fingolfin
Copy link
Contributor Author

I removed more parts of this PR. I hope the remaining bits will work fine. Perhaps it is best if I wait until this PR is merged, and then re-submit the bits I removed as separate PRs.

fchapoton
fchapoton approved these changes Jun 19, 2023
View reviewed changes
Copy link
Contributor

@fchapoton fchapoton left a comment

Choose a reason for hiding this comment

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

gut, danke schoen

@vbraun
Copy link
Member

vbraun commented Jun 21, 2023

merge conflict

@fingolfin
Copy link
Contributor Author

rebased

@github-actions
Copy link

Documentation preview for this PR (built with commit fb0908e; changes) is ready! 🎉

fchapoton
fchapoton approved these changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@fchapoton fchapoton left a comment

Choose a reason for hiding this comment

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

ok again

This was referenced Jun 30, 2023
Open
Merged
Merged
@vbraun vbraun merged commit 615fbb8 into sagemath:develop Jul 1, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jul 1, 2023
@fingolfin fingolfin deleted the mh/more-libgap branch July 2, 2023 09:58
vbraun pushed a commit that referenced this pull request Jul 9, 2023
gh-35863: gap: stop using T_CHAR
e66446b 
    
Contains PR #35761, once that is merged I'll rebase here.
    
URL: #35863
Reported by: Max Horn
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit that referenced this pull request Aug 5, 2023
gh-35862: gap: revise make_gap_list, make_gap_matrix
a1a27b1 
    
Contains PR #35761, once that is merged I'll rebase here.
    
URL: #35862
Reported by: Max Horn
Reviewer(s): Frédéric Chapoton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcoudert dcoudert dcoudert requested changes

@fchapoton fchapoton fchapoton approved these changes

Assignees
No one assigned
Labels
c: packages: standard
Projects
None yet
Milestone
sage-10.1
Development

Successfully merging this pull request may close these issues.
5 participants
@fingolfin @vbraun @fchapoton @dcoudert @mkoeppe