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.

Sign up for GitHub

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

Jump to bottom

Make ptr::eq documentation mention fat-pointer behavior #59390

Merged
merged 4 commits into from
Mar 28, 2019
Merged

Make ptr::eq documentation mention fat-pointer behavior #59390

merged 4 commits into from
Mar 28, 2019

Conversation

czipperz
Copy link
Contributor

Resolves #59214

@rust-highfive
Copy link
Collaborator

r? @Kimundi

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 24, 2019
@CryZe
Copy link
Contributor

CryZe commented Mar 25, 2019 •
edited
Loading

I'm pretty sure this is incorrect. The difference is that fat pointers (not smart pointers) compare their metadata in addition to the pointer address (no value comparison is done either, there is no Eq / PartialEq trait bound).

@steveklabnik
Copy link
Member

Yes, I agree with @CryZe

@czipperz
Copy link
Contributor Author

What do you mean by a fat pointer? Why is smart pointer wrong?

The three example smart pointers I listed compare by value. They use compare by address only as an optimization when the type implements Eq according to reading the source code.

@czipperz
Copy link
Contributor Author

Googled fat pointers:

  • slices with length
  • traits with vtables

@czipperz
Copy link
Contributor Author

KK so the issue is wrong. I'll fix this. Thanks guys

@czipperz
Copy link
Contributor Author

Should we be adding a method that compares pointers by purely memory address without comparing metadata?

@czipperz
Copy link
Contributor Author

@steveklabnik

@czipperz czipperz changed the title Make ptr::eq documentation mention smart-pointer behavior Make ptr::eq documentation mention fat-pointer behavior Mar 26, 2019
@steveklabnik
Copy link
Member

@bors: r+ rollup

thank you!

@bors
Copy link
Contributor

bors commented Mar 27, 2019

📌 Commit fbfc808 has been approved by steveklabnik

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 27, 2019
Centril
Centril suggested changes Mar 27, 2019
View reviewed changes
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

We shouldn't be using Trait instead of dyn Trait anymore in the standard library.

src/libcore/ptr.rs Outdated Show resolved Hide resolved
src/libcore/ptr.rs Outdated Show resolved Hide resolved
src/libcore/ptr.rs Outdated Show resolved Hide resolved
src/libcore/ptr.rs Outdated Show resolved Hide resolved
src/libcore/ptr.rs Outdated Show resolved Hide resolved
src/libcore/ptr.rs Outdated Show resolved Hide resolved
src/libcore/ptr.rs Outdated Show resolved Hide resolved
src/libcore/ptr.rs Outdated Show resolved Hide resolved
src/libcore/ptr.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Mar 27, 2019

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 27, 2019
@Centril
Copy link
Contributor

Centril commented Mar 27, 2019

r? @steveklabnik

@czipperz
Copy link
Contributor Author

KK @Centril done

@Centril
Copy link
Contributor

Centril commented Mar 27, 2019

r=me,steveklabnik with green travis

@czipperz
Copy link
Contributor Author

@Centril @steveklabnik

@Centril
Copy link
Contributor

Centril commented Mar 27, 2019

@bors r=Centril,steveklabnik rollup

@bors
Copy link
Contributor

bors commented Mar 27, 2019

📌 Commit 61b6c56 has been approved by Centril,steveklabnik

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 27, 2019
cuviper added a commit to cuviper/rust that referenced this pull request Mar 28, 2019
@cuviper
Rollup merge of rust-lang#59390 - czipperz:ptr_eq_smart_pointer, r=Ce…
35b339b 
…ntril,steveklabnik

Make `ptr::eq` documentation mention fat-pointer behavior

Resolves rust-lang#59214
@cuviper cuviper mentioned this pull request Mar 28, 2019
Merged
bors added a commit that referenced this pull request Mar 28, 2019
@bors
Auto merge of #59471 - cuviper:rollup, r=cuviper
d20e000 
Rollup of 18 pull requests

Successful merges:

 - #57293 (Make some lints incremental)
 - #57565 (syntax: Remove warning for unnecessary path disambiguators)
 - #58253 (librustc_driver => 2018)
 - #58837 (librustc_interface => 2018)
 - #59268 (Add suggestion to use `&*var` when `&str: From<String>` is expected)
 - #59283 (Make ASCII case conversions more than 4× faster)
 - #59284 (adjust MaybeUninit API to discussions)
 - #59372 (add rustfix-able suggestions to trim_{left,right} deprecations)
 - #59390 (Make `ptr::eq` documentation mention fat-pointer behavior)
 - #59393 (Refactor tuple comparison tests)
 - #59420 ([CI] record docker image info for reuse)
 - #59421 (Reject integer suffix when tuple indexing)
 - #59430 (Renames `EvalContext` to `InterpretCx`)
 - #59439 (Generalize diagnostic for `x = y` where `bool` is the expected type)
 - #59449 (fix: Make incremental artifact deletion more robust)
 - #59451 (Add `Default` to `std::alloc::System`)
 - #59459 (Add some tests)
 - #59460 (Include id in Thread's Debug implementation)

Failed merges:

r? @ghost
@bors bors merged commit 61b6c56 into rust-lang:master Mar 28, 2019
@RalfJung RalfJung mentioned this pull request Oct 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centril Centril Centril approved these changes

Assignees

@steveklabnik steveklabnik

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@czipperz @rust-highfive @CryZe @steveklabnik @bors @Centril @Kimundi