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

replaced scoped blocks #18

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

replaced scoped blocks #18

wants to merge 1 commit into from

Conversation

joroec
Copy link
Owner

@joroec joroec commented Aug 28, 2019

The scoped blocks were replaced by anonymous functions, since defer calls are only triggered when leaving a function frame and not a scope.

@joroec joroec added the bug Something isn't working label Aug 28, 2019
@joroec joroec requested a review from obitech August 28, 2019 09:06
Copy link
Collaborator

@obitech obitech left a comment

Choose a reason for hiding this comment

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

I don't really understand the point of using anonymous functions for defer here. All of them are at the end of their respective encapsulating functions, so why bother with a new stack frame? defer would be called in any case if the upper function returns right? I haven't quite grasped the need for the additonal nesting to be honest.

Alternatively, maybe you can layout the expected behaviour vs. what's actually happening in:

  • no scoping
  • scoped blocks (used previously)
  • anonymous functions

continue
}
// anonymous function for safely calling defer
func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really understand why this has to be an anonymous function. The function is returning after this whole block anyway, so there shouldn't be any issues with calling ?

Base automatically changed from master to main March 3, 2021 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants