diff options
author | Jonathan Hefner <jonathan@hefner.pro> | 2019-05-05 13:19:09 -0500 |
---|---|---|
committer | Jonathan Hefner <jonathan@hefner.pro> | 2019-05-05 13:48:05 -0500 |
commit | 0e2de0e3fdc9a1fc763531b74e9fc49666022ff9 (patch) | |
tree | 283b765f796f0c2fe34715b400aa3e41ebc34929 | |
parent | 9b0e632def6a66bbd0c0aba8531b2a173f7c3064 (diff) | |
download | rails-0e2de0e3fdc9a1fc763531b74e9fc49666022ff9.tar.gz rails-0e2de0e3fdc9a1fc763531b74e9fc49666022ff9.tar.bz2 rails-0e2de0e3fdc9a1fc763531b74e9fc49666022ff9.zip |
Improve String#first and #last performance
Removes unnecessary conditional and method call for significant
performance improvement. As a side effect, this fixes an unexpected
behavior where passing a limit of 0 would return a frozen string.
This also implements the Rails 6.1 intended behavior with regards to
negative limits, and removes the previous deprecation warnings.
String#first Comparison:
new: 3056515.0 i/s
old: 1943310.2 i/s - 1.57x slower
String#last Comparison:
new: 2691919.0 i/s
old: 1924256.6 i/s - 1.40x slower
(Note: "old" benchmarks have deprecation warnings commented out, for a
more fair comparison.)
-rw-r--r-- | activesupport/lib/active_support/core_ext/string/access.rb | 24 | ||||
-rw-r--r-- | activesupport/test/core_ext/string_ext_test.rb | 32 |
2 files changed, 22 insertions, 34 deletions
diff --git a/activesupport/lib/active_support/core_ext/string/access.rb b/activesupport/lib/active_support/core_ext/string/access.rb index 4ca24028b0..a0a7c54410 100644 --- a/activesupport/lib/active_support/core_ext/string/access.rb +++ b/activesupport/lib/active_support/core_ext/string/access.rb @@ -75,17 +75,7 @@ class String # str.first(0) # => "" # str.first(6) # => "hello" def first(limit = 1) - ActiveSupport::Deprecation.warn( - "Calling String#first with a negative integer limit " \ - "will raise an ArgumentError in Rails 6.1." - ) if limit < 0 - if limit == 0 - "" - elsif limit >= size - dup - else - to(limit - 1) - end + self[0, limit] || (raise ArgumentError, "negative limit") end # Returns the last character of the string. If a limit is supplied, returns a substring @@ -99,16 +89,6 @@ class String # str.last(0) # => "" # str.last(6) # => "hello" def last(limit = 1) - ActiveSupport::Deprecation.warn( - "Calling String#last with a negative integer limit " \ - "will raise an ArgumentError in Rails 6.1." - ) if limit < 0 - if limit == 0 - "" - elsif limit >= size - dup - else - from(-limit) - end + self[[length - limit, 0].max, limit] || (raise ArgumentError, "negative limit") end end diff --git a/activesupport/test/core_ext/string_ext_test.rb b/activesupport/test/core_ext/string_ext_test.rb index c5a000b67a..b83381bdb2 100644 --- a/activesupport/test/core_ext/string_ext_test.rb +++ b/activesupport/test/core_ext/string_ext_test.rb @@ -480,12 +480,16 @@ class StringAccessTest < ActiveSupport::TestCase assert_not_same different_string, string end - test "#first with negative Integer is deprecated" do - string = "hello" - message = "Calling String#first with a negative integer limit " \ - "will raise an ArgumentError in Rails 6.1." - assert_deprecated(message) do - string.first(-1) + test "#first with Integer returns a non-frozen string" do + string = "he" + (0..string.length + 1).each do |limit| + assert_not string.first(limit).frozen? + end + end + + test "#first with negative Integer raises ArgumentError" do + assert_raise ArgumentError do + "hello".first(-1) end end @@ -507,12 +511,16 @@ class StringAccessTest < ActiveSupport::TestCase assert_not_same different_string, string end - test "#last with negative Integer is deprecated" do - string = "hello" - message = "Calling String#last with a negative integer limit " \ - "will raise an ArgumentError in Rails 6.1." - assert_deprecated(message) do - string.last(-1) + test "#last with Integer returns a non-frozen string" do + string = "he" + (0..string.length + 1).each do |limit| + assert_not string.last(limit).frozen? + end + end + + test "#last with negative Integer raises ArgumentError" do + assert_raise ArgumentError do + "hello".last(-1) end end |