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

Fix GcUnsafe2 #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix GcUnsafe2 #7

wants to merge 1 commit into from

Conversation

pb-cdunn
Copy link

There are tons of GcUnsafe2 warnings now, using nim-HEAD:

nim-heap/binaryheap.nim(45, 4) Warning: 'propFulfilled' is not GC-safe as it performs an indirect call here [GcUnsafe2]
 h.comp(h.data[indParent], h.data[indChild]) <= 0
  ^
nim-heap/binaryheap.nim(86, 6) Warning: 'siftdown' is not GC-safe as it calls 'propFulfilled' [GcUnsafe2]
...

There are tons of GcUnsafe2 warnings now, using nim-HEAD:

```
nim-heap/binaryheap.nim(45, 4) Warning: 'propFulfilled' is not GC-safe as it performs an indirect call here [GcUnsafe2]
 h.comp(h.data[indParent], h.data[indChild]) <= 0
  ^
nim-heap/binaryheap.nim(86, 6) Warning: 'siftdown' is not GC-safe as it calls 'propFulfilled' [GcUnsafe2]
...
```
cdunn2001 pushed a commit to bio-nim/nim-falcon that referenced this pull request May 24, 2019
cdunn2001 pushed a commit to bio-nim/nim-falcon that referenced this pull request May 24, 2019
The "{.gcsafe.}" change might not be needed. The real fix for that
was bluenote10/nim-heap#7
@bluenote10
Copy link
Owner

Thanks for looking into this. Compilation on travis failed, did it compile for you with the change?

Do we have to change the other occurrences of the comp signature as well? Perhaps we can simply introduce a type definition for the callback and use it everywhere.

@pb-cdunn
Copy link
Author

pb-cdunn commented Jun 7, 2019

Compilation on travis failed, did it compile for you with the change?

Actually, no. Your test fails me for with this change. And system.cmp is common, so your test is a good one. I'll have to revisit this someday, when I have time.

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