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

Do not allow memoization methods with nonzero arity #10

Merged
merged 4 commits into from Jan 10, 2013

Conversation

mbj
Copy link
Collaborator

@mbj mbj commented Jan 9, 2013

This makes sure the memoizer is not used to create tricky to track down
bugs.

Currently adamantium allows the following:

class Foo
  include Adamantium

  def something(x)
    x * 2
  end
  memoize :something
end

foo = Foo.new
foo.something(1) # => 2
foo.something(2) # => 2, Tricky to track down bug.

Markus Schirp added 3 commits January 9, 2013 16:15
This makes sure the memoizer is not used to create tricky to track down
bugs.
Dunno how we will kill the mutation under 1.9 as soon mutant is able to
mutate adamantium.
@mbj
Copy link
Collaborator Author

mbj commented Jan 9, 2013

@dkubb Any problems with this? I'd like to get it merged.

visibility = method_visibility(method)
def memoize_method(method_name, freezer)
method = instance_method(method_name)
unless method.arity.zero?
Copy link
Owner

Choose a reason for hiding this comment

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

You could also write this as if method.arity.nonzero? which matches what you're testing a bit more closely.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, since you're only using method one time I would be more likely just to do it inline, eg:

if instance_method(method_name).arity.nonzero?
  # ...

EDIT: oh wait, I see what you're doing now. nevermind. Does UnboundMethod#name always know the original name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dkubb
Copy link
Owner

dkubb commented Jan 10, 2013

@mbj I think you can merge this in if you change to use Numeric#nonzero? like I suggested.

@mbj
Copy link
Collaborator Author

mbj commented Jan 10, 2013

CI failed for external and IMHO temporal reason on one ruby https://travis-ci.org/dkubb/adamantium/builds/4056359 I'll merge anyways. This will trigger a new build that I predict to be green.

mbj pushed a commit that referenced this pull request Jan 10, 2013
Do not allow memoization methods with nonzero arity
@mbj mbj merged commit 834d918 into dkubb:master Jan 10, 2013
@dkubb
Copy link
Owner

dkubb commented Jan 10, 2013

@mbj when a single build fails you can always restart it by clicking on the icon in the upper-right that looks like a little gear.. click on "Restart Job".

@mbj
Copy link
Collaborator Author

mbj commented Jan 10, 2013

Thx. Im not a great GUI user ;). BTW I plan to add a rake task to devtools for this job. (Or a CLI task)

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