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

Remove unused var, replace temps and block.call with yield #83

Closed
wants to merge 1 commit into from

Conversation

ridiculous
Copy link

No description provided.

end
end

if block_given?
if block.arity == 1
block.call(self)
yield self
Copy link

Choose a reason for hiding this comment

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

To actually get a memory/performance benefit from this change you also need to remove the &block in the definition of this method.

Copy link
Author

Choose a reason for hiding this comment

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

we won't get the memory savings because we need block on line 26. But we should see a slight increase in speed.

@dirk
Copy link

dirk commented Oct 7, 2015

So @ridiculous has a good set of changes here.

@cyu: Could you please take some time to review this?

@cyu
Copy link
Owner

cyu commented Oct 7, 2015

I still need a block to check the arity

@ridiculous
Copy link
Author

Should I revert the block.call part and make this only about removing the vars?

@dirk
Copy link

dirk commented Oct 8, 2015

@cyu: Investigating the arity stuff to see if we can get rid of &block while still keeping the same functionality.

@dirk
Copy link

dirk commented Oct 10, 2015

@ridiculous: So I investigated this, and Proc.new() does contain arity information about the passed block. The following should therefore work for the code that @cyu was concerned about:

      if block_given?
        block = Proc.new
        if block.arity == 1
          block.call self
        else
          instance_eval(&block)
        end
      end

@cyu
Copy link
Owner

cyu commented Oct 10, 2015

@dirk the block_given? == false use case is so rare that I don't think there's a lot to gain from this optimization. And I can't imagine there being a large advantage from using &block verses Proc.new in terms of performance.

Having said that, the block.call self execution path is there to support an older configuration method that really isn't documented anymore - we can probably remove that (obsoleting the need for the arity call) which will allow us to use Proc.new instead of &block. I would then incorporate this as a major version change.

@ridiculous
Copy link
Author

Is there a change that is being requested on this PR?

@ridiculous ridiculous closed this Jun 2, 2016
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.

None yet

3 participants