Skip to content

Conversation

@fingolfin
Copy link
Member

... as recommended by https://docs.julialang.org/en/v1/manual/embedding/index.html

This makes it possible to collect coverage data in Julia code (as that is written to disk via that atexit hook).

@fingolfin fingolfin added topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes backport-to-4.10 labels Oct 18, 2018
@ChrisJefferson
Copy link
Contributor

The changes to Panic/SyExit are sensible and good.

One random question -- why doesn't julia_gc just pass jl_atexit_hook to atexit?

Copy link
Member

@sebasguts sebasguts left a comment

Choose a reason for hiding this comment

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

This looks sensible

src/saveload.c Outdated
{
Pr("Bad type %d, size %d\n",type,size);
exit(1);
Panic("Bad type %d, size %d\n",type,size);
Copy link
Member

Choose a reason for hiding this comment

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

The compiler seems to complain about the %d

@fingolfin
Copy link
Member Author

@ChrisJefferson fair question, I also wondered. One thing is that jl_atexit_hook expects the exit code as an argument, which is (AFAIK) unknown in regular atexit handlers. I am not aware of other reasons, but that doesn't mean there aren't any. I'll try to find out.

@ChrisJefferson
Copy link
Contributor

Wanting the exit code is a good enough reason, in that case I'm happy to merge it (it's not going to effect GAP without Julia anyway of course)

@fingolfin fingolfin merged commit 9d49231 into gap-system:master Oct 19, 2018
@fingolfin fingolfin deleted the mh/jl_atexit_hook branch October 19, 2018 14:08
@fingolfin
Copy link
Member Author

Backported via 19aafe0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants