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

keep the order of the callbacks #9642

Merged
merged 1 commit into from
Nov 7, 2018
Merged

Conversation

narimiran
Copy link
Member

Put new callbacks at the end of the CallbackList, not in front, so the order of callbacks is not reversed.

Fixes #7197.

@Araq Araq requested a review from dom96 November 7, 2018 18:49
@dom96 dom96 merged commit d0a02fe into nim-lang:devel Nov 7, 2018
@krux02
Copy link
Contributor

krux02 commented Nov 7, 2018

I just want to mention that atexit reverse the order of callbacks for good reasons. They reverse it, because they are often used to register destructors in C, and here it is important that the destruction is done in reverse order.

@dom96
Copy link
Contributor

dom96 commented Nov 8, 2018

Honestly I'm not sure why it's such a big deal that the callbacks were reversed, but shrug. I didn't want multiple callbacks on futures in the first place.

@Araq
Copy link
Member

Araq commented Nov 8, 2018

here it is important that the destruction is done in reverse order.

Not really, but it also doesn't hurt. Quick analysis:

Destruction calls dealloc. Order irrelevant.
Destruction calls close on a handle. Order irrelevant.
Destruction calls release on a lock. Order relevant but omg, who does that?!

@krux02
Copy link
Contributor

krux02 commented Nov 8, 2018

well the order is relevant when the destructor wants to close a file that is stored in a member of an object that has already been deallocated.

@Araq
Copy link
Member

Araq commented Nov 8, 2018

That would have been a single atexit handler then, not 2. Assuming somewhat sane programming practices.

@narimiran narimiran deleted the callback-order branch November 12, 2018 15:16
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.

4 participants