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

Remove an unused local variable canBlock #706

Merged
merged 1 commit into from Nov 2, 2017

Conversation

Projects
None yet
3 participants
@hajimehoshi
Member

hajimehoshi commented Oct 7, 2017

This PR removes canBlock property from $curGoroutine, that is not used anywhere.

@hajimehoshi hajimehoshi changed the title from Remove canBlock to Remove an unused local variable canBlock Oct 7, 2017

@dmitshur

dmitshur approved these changes Oct 12, 2017 edited

I investigated the history behind this. It looks like canBlock was previously used, but its use was removed in 2d0acd0.

So, this has my LGTM. I don't see anything wrong, and CI passes.

@neelance Any objections to this being merged?

@hajimehoshi

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi
Member

hajimehoshi commented Oct 20, 2017

ping @neelance ?

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 2, 2017

Member

For those how know JavaScript better than I do, is there any difference in behavior between these two snippets of code?

try {
    var result = v.apply(passThis ? this : undefined, args);
} finally {}

vs

var result = v.apply(passThis ? this : undefined, args);

That is the only part of this change that I'm not fully confident about.

Member

dmitshur commented Nov 2, 2017

For those how know JavaScript better than I do, is there any difference in behavior between these two snippets of code?

try {
    var result = v.apply(passThis ? this : undefined, args);
} finally {}

vs

var result = v.apply(passThis ? this : undefined, args);

That is the only part of this change that I'm not fully confident about.

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Nov 2, 2017

Member

@shurcooL From my knowledge:

  1. If the snippets are exactly as you wrote, then no, there is not a difference between their behavior.
  2. If the finally {} block actually has some code inside of it, then yes. Statements inside of finally execute regardless of whether or not the try code succeeded (like how a function in Go can return an error but defer code still runs).

You can read more here if you are interested: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch

Member

slimsag commented Nov 2, 2017

@shurcooL From my knowledge:

  1. If the snippets are exactly as you wrote, then no, there is not a difference between their behavior.
  2. If the finally {} block actually has some code inside of it, then yes. Statements inside of finally execute regardless of whether or not the try code succeeded (like how a function in Go can return an error but defer code still runs).

You can read more here if you are interested: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 2, 2017

Member

Thanks @slimsag, that helps me understand this change better.

The finally block being removed here has code $curGoroutine.canBlock = canBlock;, but removing that should be safe since canBlock is not used anywhere else (it was removed in 2d0acd0), so changing its value has no effect.

I just wanted to make sure there's no other effect to having code be in a try/finally block (e.g., in case of exceptions or something).

Member

dmitshur commented Nov 2, 2017

Thanks @slimsag, that helps me understand this change better.

The finally block being removed here has code $curGoroutine.canBlock = canBlock;, but removing that should be safe since canBlock is not used anywhere else (it was removed in 2d0acd0), so changing its value has no effect.

I just wanted to make sure there's no other effect to having code be in a try/finally block (e.g., in case of exceptions or something).

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 2, 2017

Member

The var result = v.apply(...); line was not inside a try block originally. It got placed in one only when the local canBlock variable was introduced in 6e21f51.

So, given canBlock is no longer used anywhere else, it should be safe to remove the try block and go back to a simple v.apply call.

There have been no objections from @neelance after some time, and I'm quite confident of this being correct now, so merging. Thanks again @hajimehoshi and @slimsag!

Member

dmitshur commented Nov 2, 2017

The var result = v.apply(...); line was not inside a try block originally. It got placed in one only when the local canBlock variable was introduced in 6e21f51.

So, given canBlock is no longer used anywhere else, it should be safe to remove the try block and go back to a simple v.apply call.

There have been no objections from @neelance after some time, and I'm quite confident of this being correct now, so merging. Thanks again @hajimehoshi and @slimsag!

@dmitshur dmitshur merged commit 444abdf into gopherjs:master Nov 2, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@hajimehoshi hajimehoshi deleted the hajimehoshi:canblock branch Mar 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment