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

Add PartialEq and Eq to Cursor #67233

Merged
merged 1 commit into from Dec 23, 2019
Merged

Add PartialEq and Eq to Cursor #67233

merged 1 commit into from Dec 23, 2019

Conversation

@Luro02
Copy link
Contributor

Luro02 commented Dec 11, 2019

closes #67226

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 11, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 11, 2019

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-12-11T17:02:21.3386446Z ========================== Starting Command Output ===========================
2019-12-11T17:02:21.3387563Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/260f8301-421f-4d8e-96ec-2ebd11c37aa9.sh
2019-12-11T17:02:21.3387594Z 
2019-12-11T17:02:21.3390024Z ##[section]Finishing: Disable git automatic line ending conversion
2019-12-11T17:02:21.3395221Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/67233/merge to s
2019-12-11T17:02:21.3396555Z Task         : Get sources
2019-12-11T17:02:21.3396622Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2019-12-11T17:02:21.3396646Z Version      : 1.0.0
2019-12-11T17:02:21.3396670Z Author       : Microsoft
---
2019-12-11T17:02:23.0426855Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-11T17:02:23.0598291Z ##[command]git config gc.auto 0
2019-12-11T17:02:23.0670176Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-11T17:02:23.0711702Z ##[command]git config --get-all http.proxy
2019-12-11T17:02:23.0832708Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67233/merge:refs/remotes/pull/67233/merge
---
2019-12-11T17:06:45.7085495Z     Checking panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
2019-12-11T17:06:49.5511965Z error: impl has missing stability attribute
2019-12-11T17:06:49.5512933Z    --> src/libstd/io/cursor.rs:394:1
2019-12-11T17:06:49.5513430Z     |
2019-12-11T17:06:49.5513932Z 394 | / impl<T: PartialEq> PartialEq for Cursor<T> {
2019-12-11T17:06:49.5514442Z 395 | |     fn eq(&self, other: &Self) -> bool {
2019-12-11T17:06:49.5515010Z 396 | |         self.get_ref() == other.get_ref() && self.pos == other.pos
2019-12-11T17:06:49.5515945Z 398 | | }
2019-12-11T17:06:49.5516363Z     | |_^
2019-12-11T17:06:49.5516607Z 
2019-12-11T17:06:49.5517011Z error: impl has missing stability attribute
2019-12-11T17:06:49.5517011Z error: impl has missing stability attribute
2019-12-11T17:06:49.5517430Z    --> src/libstd/io/cursor.rs:400:1
2019-12-11T17:06:49.5517847Z     |
2019-12-11T17:06:49.5518281Z 400 | impl<T: Eq> Eq for Cursor<T> {}
2019-12-11T17:06:49.5518960Z 
2019-12-11T17:06:49.6596764Z error: aborting due to 2 previous errors
2019-12-11T17:06:50.2098810Z 
2019-12-11T17:06:50.2099830Z error: could not compile `std`.
---
2019-12-11T17:06:50.2101632Z   local time: Wed Dec 11 17:06:49 UTC 2019
2019-12-11T17:06:50.2101808Z   network time: Wed, 11 Dec 2019 17:06:49 GMT
2019-12-11T17:06:50.2102025Z == end clock drift check ==
2019-12-11T17:06:52.5516450Z 
2019-12-11T17:06:52.5592288Z ##[error]Bash exited with code '1'.
2019-12-11T17:06:52.5602079Z ##[section]Finishing: Run build
2019-12-11T17:06:52.5614744Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/67233/merge to s
2019-12-11T17:06:52.5616269Z Task         : Get sources
2019-12-11T17:06:52.5616304Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2019-12-11T17:06:52.5616341Z Version      : 1.0.0
2019-12-11T17:06:52.5616389Z Author       : Microsoft
2019-12-11T17:06:52.5616389Z Author       : Microsoft
2019-12-11T17:06:52.5616425Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2019-12-11T17:06:52.5616585Z ==============================================================================
2019-12-11T17:06:52.8680867Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2019-12-11T17:06:52.8711608Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/67233/merge to s
2019-12-11T17:06:52.8797443Z Start cleaning up orphan processes.
2019-12-11T17:06:52.8877797Z Terminate orphan process: pid (5159) (python)
2019-12-11T17:06:52.9018043Z ##[section]Finishing: Finalize Job
2019-12-11T17:06:52.9063377Z ##[section]Finishing: Linux mingw-check

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

src/libstd/io/cursor.rs Outdated Show resolved Hide resolved
@sfackler sfackler added the T-libs label Dec 12, 2019
@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Dec 12, 2019

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 12, 2019

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 12, 2019

Could you give an example of how you’d use this and why it’s useful? If T is a slice or Vec of bytes, isn’t it expensive to compare the entire thing?

@Centril Centril added the relnotes label Dec 12, 2019
@Centril Centril added this to the 1.42 milestone Dec 12, 2019
@Luro02

This comment has been minimized.

Copy link
Contributor Author

Luro02 commented Dec 12, 2019

I personally need it for testing. It is easier to read

assert_eq!(Cursor::new(vec![2, 3, 4]), Cursor::new(vec![2, 3, 4]));

than

assert_eq!(Cursor::new(vec![2, 3, 4]).get_ref(), Cursor::new(vec![2, 3, 4]).get_ref());

In my case I have a struct, that wraps around a reader, where it only implements PartialEq if the underlying reader implements it. So I can't assert my struct, because it has an underlying Cursor, which doesn't implement PartialEq.

This is the wrapper I am talking about:
https://github.com/Luro02/sub_cursor

PartialEq for Mutex is WIP...

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 12, 2019

Could this also be defined with #[derive] since I think the implementation is the same?

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 12, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 21, 2019

Please squash the commits into one. After that, I think this'll be ready for approval (modulo FCP concluding in ~1 day).

@Luro02

This comment has been minimized.

Copy link
Contributor Author

Luro02 commented Dec 21, 2019

Could you tell me how to do this correctly please? (I never did this)

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 21, 2019

I recommend git rebase -i origin/master (or whatever your "upstream" remote is instead of origin, the one that corresponds to rust-lang/rust). You'll get a listing of commits and all but the first one can be changed to have "fixup" as the first word (instead of "pick") and when you then save that it'll create a single commit with the contents of all 3 squashed into one.

Let me know if you need further help!

@Luro02 Luro02 force-pushed the Luro02:cursor_traits branch from 46f5d91 to 89986a3 Dec 22, 2019
@Luro02

This comment has been minimized.

Copy link
Contributor Author

Luro02 commented Dec 22, 2019

@Mark-Simulacrum Thank you for your friendly and helpful response :)

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 22, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Dec 22, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 22, 2019

📌 Commit 89986a3 has been approved by sfackler

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 22, 2019

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

Centril added a commit to Centril/rust that referenced this pull request Dec 23, 2019
Add PartialEq and Eq to Cursor

closes rust-lang#67226
bors added a commit that referenced this pull request Dec 23, 2019
Rollup of 5 pull requests

Successful merges:

 - #66913 (Suggest calling method when first argument is `self`)
 - #67233 (Add PartialEq and Eq to Cursor)
 - #67527 (Results show too much)
 - #67543 (Add regression tests for fixed ICEs)
 - #67546 (Fix ICE in mir interpretation)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Dec 23, 2019
Add PartialEq and Eq to Cursor

closes rust-lang#67226
bors added a commit that referenced this pull request Dec 23, 2019
Rollup of 4 pull requests

Successful merges:

 - #67233 (Add PartialEq and Eq to Cursor)
 - #67527 (Results show too much)
 - #67543 (Add regression tests for fixed ICEs)
 - #67546 (Fix ICE in mir interpretation)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Dec 23, 2019
Add PartialEq and Eq to Cursor

closes rust-lang#67226
bors added a commit that referenced this pull request Dec 23, 2019
Rollup of 8 pull requests

Successful merges:

 - #67233 (Add PartialEq and Eq to Cursor)
 - #67466 (Require const stability attributes on intrinsics to be able to use them in constant contexts)
 - #67507 (Remove mem::uninitalized from tests)
 - #67527 (Results show too much)
 - #67536 (Move `{hir::lowering -> hir}::is_range_literal`)
 - #67538 (Improve diagnostics for invalid assignment)
 - #67546 (Fix ICE in mir interpretation)
 - #67559 (Document that calling Drop, even after it panics, is UB)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Dec 23, 2019
Rollup of 8 pull requests

Successful merges:

 - #67233 (Add PartialEq and Eq to Cursor)
 - #67466 (Require const stability attributes on intrinsics to be able to use them in constant contexts)
 - #67507 (Remove mem::uninitalized from tests)
 - #67527 (Results show too much)
 - #67536 (Move `{hir::lowering -> hir}::is_range_literal`)
 - #67538 (Improve diagnostics for invalid assignment)
 - #67546 (Fix ICE in mir interpretation)
 - #67559 (Document that calling Drop, even after it panics, is UB)

Failed merges:

r? @ghost
@bors bors merged commit 89986a3 into rust-lang:master Dec 23, 2019
4 checks passed
4 checks passed
pr Build #20191222.14 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.