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

Delegate debug implementation for InternedString #5184

Merged
merged 1 commit into from
Mar 15, 2018

Conversation

matklad
Copy link
Member

@matklad matklad commented Mar 15, 2018

Let's make InternedString debug implementation the same as for String / str? It's more concise, which helps when you debug printing other stuff, like eprintln!("id = {:?}", a_package_id).

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@matklad
Copy link
Member Author

matklad commented Mar 15, 2018

@bors delegate=Eh2406

cc @Eh2406 :)

@bors
Copy link
Contributor

bors commented Mar 15, 2018

✌️ @Eh2406 can now approve this pull request

@Eh2406
Copy link
Contributor

Eh2406 commented Mar 15, 2018

I originally added the extra verbiage to help debugging cases were InternedString and str act different. But now that they hash the same, I don't know of any place that will get past the type checker.
@bors r+

@bors
Copy link
Contributor

bors commented Mar 15, 2018

📌 Commit 111024c has been approved by Eh2406

@bors
Copy link
Contributor

bors commented Mar 15, 2018

⌛ Testing commit 111024c with merge 9895340...

bors added a commit that referenced this pull request Mar 15, 2018
Delegate debug implementation for InternedString

Let's make `InternedString` debug implementation the same as for `String` / `str`? It's more concise, which helps when you debug printing other stuff, like `eprintln!("id = {:?}", a_package_id)`.
@matklad
Copy link
Member Author

matklad commented Mar 15, 2018

Hm, @Eh2406, could we implement InternedString as pub struct InternedString(&'static str)? That should allow us to get rid of some unsafe blocks here perhaps?

@Eh2406
Copy link
Contributor

Eh2406 commented Mar 15, 2018

Yes it could, and I'd be open to a PR that does.
It was done this way to have things by default be the fast pointer comparison rather than the full string comparison.
But:

  • Eq I believe already has a fast path for the pointers being equal.
  • Hash we had to back out, apparently cargo requires things to Hash the same across program executions.
  • !Eq we know that interned strings have been duplicated, if the pointers differ than the contents differ.

So we would switch from overriding hash to overriding EQ.

@matklad
Copy link
Member Author

matklad commented Mar 15, 2018

It was done this way to have things by default be the fast pointer comparison rather than the full string comparison.

I think this is could be implemented if we store &static str and implement PartialEq manually: https://play.rust-lang.org/?gist=b55374c5e2d04aef28d385f5bee22ec1&version=stable

@bors
Copy link
Contributor

bors commented Mar 15, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: Eh2406
Pushing 9895340 to master...

@bors bors merged commit 111024c into rust-lang:master Mar 15, 2018
@matklad matklad deleted the delegate-debug branch March 15, 2018 12:26
@Eh2406
Copy link
Contributor

Eh2406 commented Mar 15, 2018

Completely agree, that would work.

@ehuss ehuss added this to the 1.26.0 milestone Feb 6, 2022
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.

6 participants