Skip to content

[Ruby 2.5] String#-@ - #586

Merged
eregon merged 3 commits into
masterfrom
string#-@-deduplicates-unfrozen-strings
Jan 27, 2018
Merged

[Ruby 2.5] String#-@#586
eregon merged 3 commits into
masterfrom
string#-@-deduplicates-unfrozen-strings

Conversation

@andrykonchin

@andrykonchinandrykonchin commented Jan 26, 2018

Copy link
Copy Markdown
Member

Done:

  • String#-@ deduplicates unfrozen strings. Already-frozen strings remain unchanged for compatibility
  • -"literal" (String#-@) optimized to return the same object

Issue #565

@andrykonchin
andrykonchinforce-pushed the string#-@-deduplicates-unfrozen-strings branch from 5298be9 to dda2047CompareJanuary 26, 2018 20:16
eregon
eregon approved these changes Jan 26, 2018
View reviewed changes

@eregoneregon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good 👍

eregon
eregon reviewed Jan 26, 2018
View reviewed changes
Comment threadcore/string/uminus_spec.rb

it "returns the same object when it's called on the same String literal" do
(-"unfrozen string").should equal(-"unfrozen string")
end

@eregoneregonJan 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe an extra spec to make sure it returns a different object for different strings? :D

@andrykonchinandrykonchinJan 27, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@eregon Done.
The features aren't described clearly in the tickets (IMHO) and I haven't found any related tests in the ruby/ruby git repository. So without diving into source code I cannot say these specs are comprehensive

Comment threadcore/string/uminus_spec.rb
it "returns the same object for equal unfrozen strings" do
origin = -"this string is frozen"
dynamic = -%w(this string is frozen).join(' ')

@eregoneregonJan 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be interesting to not call -@ on the dynamic part and first check origin.should_not equal dynamic.
Then (-dynamic).should equal origin.

@eregon

eregon commented Jan 26, 2018

Copy link
Copy Markdown
Member

I noticed there are several expectations like obj1.object_id.should == obj2.object_id but we already have equal matcher. It's a preferable way to determine object identity.
Is it reasonable to replace all the expectations with equal?

I think so, unless it's specifically an object_id spec. The object_id check might be a bit stronger though as equal? could be overridden (overriding object_id sounds unlikely). Best in another PR though if you want to do it.

@andrykonchin
andrykonchinforce-pushed the string#-@-deduplicates-unfrozen-strings branch from c914fbf to bca5946CompareJanuary 27, 2018 07:03
@eregon
eregon merged commit db9be93 into masterJan 27, 2018
@eregon
eregon deleted the string#-@-deduplicates-unfrozen-strings branch January 27, 2018 10:24
@eregon

eregon commented Jan 27, 2018

Copy link
Copy Markdown
Member

Thanks for the new spec!

@eregoneregon mentioned this pull request Jan 27, 2018
Closed
61 tasks
@eregon

eregon commented Jan 27, 2018

Copy link
Copy Markdown
Member

I added an extra example for frozen strings in a2fd7c4.

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

Reviewers

@eregoneregoneregon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@andrykonchin@eregon