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

Fix anonymous objects #393

Merged
merged 3 commits into from Mar 9, 2018
Merged

Fix anonymous objects #393

merged 3 commits into from Mar 9, 2018

Conversation

stereobooster
Copy link
Contributor

@stereobooster stereobooster commented Nov 9, 2017

Explanation. Trying to remove creation of anonymous objects

def test()
  "a" == "b"
end
GC.start
stat1 = GC.stat
1000_000.times do
  test
end
stat2 = GC.stat
p stat2[:count] - stat1[:count] # number of GC sweeps (minor + major)

^ This gives 147

# frozen_string_literal: true
def test()
  "a" == "b"
end
#...
p stat2[:count] - stat1[:count] # number of GC sweeps (minor + major)

^ This gives 0


def test(options = {})
end
GC.start
stat1 = GC.stat
1000_000.times do
  test
end
stat2 = GC.stat
p stat2[:count] - stat1[:count] # number of GC sweeps (minor + major) 

^ This gives 73

EMPTY_HASH = {}.freeze
def test(options = EMPTY_HASH)
end
#...
p stat2[:count] - stat1[:count] # number of GC sweeps (minor + major) 

^ This gives 0

It would be nice if ruby has magic comment for this case too, like # frozen_argument_hash: true instead of this EMPTY_HASH hack

@radar
Copy link
Collaborator

radar commented Nov 9, 2017

In the PR description could you please include a reason for these changes?

@@ -42,7 +42,7 @@ def setup
end

test "lookup: does not modify the options hash" do
options = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to revert this

@stereobooster
Copy link
Contributor Author

stereobooster commented Nov 9, 2017

Updated description. Waiting for tests to finsih

@stereobooster
Copy link
Contributor Author

Do you have any plans to do new release?

This one #392 needs more discussion.

PS. sometimes forget to do this: thank you for supporting this gem)

@@ -172,7 +174,7 @@ def translate(*args)

# Wrapper for <tt>translate</tt> that adds <tt>:raise => true</tt>. With
# this option, if no translation is found, it will raise <tt>I18n::MissingTranslationData</tt>
def translate!(key, options={})
def translate!(key, options = EMPTY_HASH)
translate(key, options.merge(:raise => true))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Calling merge on a frozen hash will not work:

irb(main):001:0> a = {}.freeze
=> {}
irb(main):002:0> a.merge!(raise: true)
RuntimeError: can't modify frozen Hash
	from (irb):2:in `merge!'
	from (irb):2
	from /Users/ryanbigg/.rubies/ruby-2.4.2/bin/irb:11:in `<main>'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

huh this means that it is not covered with tests, need to fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait a second. Yes .merge! will raise an error, because you can not mutate frozen object. But this code uses .merge without exclamation mark

@stereobooster
Copy link
Contributor Author

Sequel also uses trick with EMPTY_HASH = {}.freeze, but they name it OPTS

@radar
Copy link
Collaborator

radar commented Feb 26, 2018

@stereobooster Would you mind rebasing this PR against master? I'd like to get it merged soon rather than leave it hanging around.

@stereobooster
Copy link
Contributor Author

@radar sorry for delay. Rebased. Waiting for CI to pass

@radar
Copy link
Collaborator

radar commented Mar 9, 2018

Thank you a lot for coming back to this @stereobooster. Looks like your build is breaking because Rails master now requires Ruby 2.4+, but we were still running builds on Ruby 2.2, 2.3 for Rails master. I've fixed this now on master with 77067d6, and as long as that branch is green, your branch should be too 🤞

* master:
  Ignore Ruby 2.2.8 + 2.3.5 for Rails master
@radar radar merged commit 274ed8a into ruby-i18n:master Mar 9, 2018
@radar
Copy link
Collaborator

radar commented Mar 9, 2018

Done!

@stereobooster stereobooster deleted the anonymous-objects branch March 9, 2018 06:32
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

2 participants