13
45
Fork
You've already forked fibers
10

Fix #37 #56

Open
Reepca wants to merge 1 commit from Reepca/Fix-#37 into master
pull from: Reepca/Fix-#37
merge into: guile:master
guile:main
guile:revert-110-wip-cancel-timer-operations
guile:fdless-draft
guile:pr-60-adjust
guile:more-operations
guile:timer-wheel
guile:cross
guile:nice-dynamic-wind-experiment
Reepca commented 2022年01月30日 18:49:42 +01:00 (Migrated from github.com)
Copy link

This is the proposed fix in #37.

This is the proposed fix in #37.
aconchillo commented 2022年01月30日 19:38:08 +01:00 (Migrated from github.com)
Copy link

Thanks! This one won't probably make it to 1.1.0. I believe it makes sense but I'd like to take a closer look.

Thanks! This one won't probably make it to 1.1.0. I believe it makes sense but I'd like to take a closer look.
emixa-d commented 2022年07月23日 21:23:13 +02:00 (Migrated from github.com)
Copy link

A test case is missing, to aid with verifying whether the change works, to avoid regressions later, and to assist with verifying future changes in this area of the code.

A test case is missing, to aid with verifying whether the change works, to avoid regressions later, and to assist with verifying future changes in this area of the code.
emixa-d (Migrated from github.com) requested changes 2022年07月23日 21:33:12 +02:00
@ -128,0 +125,4 @@
(lambda ()
(catch #t
(lambda ()
(call-with-values init
emixa-d (Migrated from github.com) commented 2022年07月23日 21:29:34 +02:00
Copy link

Spacing seems wonky, though maybe that's just a different tabs / space convention (does someone know the conventions used in Fibers?).

Spacing seems wonky, though maybe that's just a different tabs / space convention (does someone know the conventions used in Fibers?).
@ -128,0 +128,4 @@
(call-with-values init
(lambda vals (atomic-box-set! ret
(list 'ok vals)))))
(lambda args
emixa-d (Migrated from github.com) commented 2022年07月23日 21:29:09 +02:00
Copy link

Don't use catch, as it destroys all backtrace information. Instead use with-exception-handler which doesn't destroy it (maybe with a fallback to 'catch' for old versions of Guile if it's not Guile 2.2). If you go for with-exception-handler, also go for raise-continuable instead of raise-exception/raise, otherwise you accumulate raise-exception in the backtrace, which is harmless but noisy.

Don't use `catch`, as it destroys all backtrace information. Instead use `with-exception-handler` which doesn't destroy it (maybe with a fallback to 'catch' for old versions of Guile if it's not Guile 2.2). If you go for `with-exception-handler`, also go for `raise-continuable` instead of `raise-exception`/`raise`, otherwise you accumulate `raise-exception` in the backtrace, which is harmless but noisy.
@ -128,0 +132,4 @@
(atomic-box-set! ret (list 'err args))
(apply throw args))))
;; Could be that this fiber was migrated away.
;; Make sure to wake up the main scheduler.
emixa-d (Migrated from github.com) commented 2022年07月23日 21:31:28 +02:00
Copy link

It doesn't make a huge difference, but for a tiny bit extra performance / less memory usage and simplicity, you could just drop the (list 'ok ...) prefix. Basically, just stuff the list in ret on success, and stuff #false in case of failure.

It doesn't make a huge difference, but for a tiny bit extra performance / less memory usage and simplicity, you could just drop the `(list 'ok ...)` prefix. Basically, just stuff the list in ret on success, and stuff `#false` in case of failure.
emixa-d (Migrated from github.com) commented 2022年07月23日 21:32:52 +02:00
Copy link

Nevermind, that's bogus because of exceptions.

Nevermind, that's bogus because of exceptions.
emixa-d (Migrated from github.com) commented 2022年07月23日 21:30:28 +02:00
Copy link

The raise-continuable I mentioned would go here.

The `raise-continuable` I mentioned would go here.
emixa-d (Migrated from github.com) reviewed 2022年07月24日 21:13:44 +02:00
@ -128,0 +132,4 @@
(atomic-box-set! ret (list 'err args))
(apply throw args))))
;; Could be that this fiber was migrated away.
;; Make sure to wake up the main scheduler.
emixa-d (Migrated from github.com) commented 2022年07月24日 21:13:44 +02:00
Copy link

Actually, if you go for with-exception-handler / raise-style exceptions, then a condition and a list can be distinguished, OTOH in principle it's allowed to raise non-conditions, so maybe let's not.

Actually, if you go for with-exception-handler / raise-style exceptions, then a condition and a list can be distinguished, OTOH in principle it's allowed to raise non-conditions, so maybe let's not.
civodul closed this pull request 2026年03月30日 20:28:12 +02:00
civodul reopened this pull request 2026年04月05日 18:05:43 +02:00

This was unintentionally closed but I see that #37 is still relevant.

@reepca Would you be willing to take another look at the reviews you got back then?

This was unintentionally closed but I see that #37 is still relevant. @reepca Would you be willing to take another look at the reviews you got back then?
This pull request is broken due to missing fork information.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin Reepca/Fix-#37:Reepca/Fix-#37
git switch Reepca/Fix-#37

Merge

Merge the changes and update on Forgejo.
git switch master
git merge --no-ff Reepca/Fix-#37
git switch Reepca/Fix-#37
git rebase master
git switch master
git merge --ff-only Reepca/Fix-#37
git switch Reepca/Fix-#37
git rebase master
git switch master
git merge --no-ff Reepca/Fix-#37
git switch master
git merge --squash Reepca/Fix-#37
git switch master
git merge --ff-only Reepca/Fix-#37
git switch master
git merge Reepca/Fix-#37
git push origin master
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
guile/fibers!56
Reference in a new issue
guile/fibers
No description provided.
Delete branch "Reepca/Fix-#37"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?