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

create a table instead of an array to hold the fibers in pmap-full #166

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

mraveloarinjaka
Copy link
Contributor

There is a crash when using pmap-full because the list of fibers passed to "join" is an array and not a table.
The fix aligns pmap-full implementation to pcall.

error: bad slot #0, expected table, got <array 0x600002C65C40>
  in table/clear [src/core/table.c] on line 411
  in drain-fibers [/Users/mraveloarinjaka/projects/sandbox/aoc2023/jpm_tree/lib/spork/ev-utils.janet] on line 35, column 3
  in join [/Users/mraveloarinjaka/projects/sandbox/aoc2023/jpm_tree/lib/spork/ev-utils.janet] on line 56, column 10
  in pmap-full [/Users/mraveloarinjaka/projects/sandbox/aoc2023/jpm_tree/lib/spork/ev-utils.janet] (tailcall) on line 84, column 3

@sogaiu
Copy link
Contributor

sogaiu commented Dec 30, 2023

That does look like a mismatch.

Using tabseq seems better and it's what pcall (which also uses join) does -- perhaps that's something that motivated the specific proposed changes :)

Just spelling things out here for other reviewers.

@mraveloarinjaka
Copy link
Contributor Author

Hello,
What are the next steps?
It is not clear whether I should amend the changes or if I am missing something.

@sogaiu
Copy link
Contributor

sogaiu commented Jan 17, 2024

My earlier comment was just saying that the proposed changes looked on the ok side. Sorry if that was unclear.

I think this PR and some others are awaiting the main maintainer's availability -- AFAIU he is pretty busy.

@bakpakin bakpakin merged commit 52d88ce into janet-lang:master Jan 17, 2024
1 check passed
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.

3 participants