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

Allow none Sized types in assert_eq! #29770

Merged
merged 2 commits into from
Nov 12, 2015
Merged

Conversation

ollie27
Copy link
Member

@ollie27 ollie27 commented Nov 11, 2015

format_args! doesn't support none Sized types so we should just pass it the references to left_val and right_val.

The following works:

assert!([1, 2, 3][..] == vec![1, 2, 3][..])

So I would expect this to as well:

assert_eq!([1, 2, 3][..], vec![1, 2, 3][..])

But it fails with "error: the trait core::marker::Sized is not implemented for the type [_] [E0277]"
I don't know if this change will have any nasty side effects I don't understand.

format_args! doesn't support none Sized types so we should just pass it the references to left_val and right_val.

This fixes `assert_eq!([1, 2, 3][..], vec![1, 2, 3][..])` for example.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (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. 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.

@bluss
Copy link
Member

bluss commented Nov 11, 2015

Does this change which type needs to impl Debug?

Imagine left_val and right_val are String, previous code:

if *left_val == *right_val {
   println!("{:?} == {:?}", *left_val, *right_val);
}

Previous code would have it compare as str and print as &str.

New code compares str and prints as &String.

The potential breaking change comes when String does not impl Debug but str does. Is my thinking around this correct? Of course String/str is fine, but if you substitute user defined types it illustrates the problem.

Workaround would be to use &*left_val instead of removing the * entirely.

@ollie27
Copy link
Member Author

ollie27 commented Nov 11, 2015

Can left_val and right_val even be Strings as they come from the following so must be references right?

match (&($left), &($right)) {
    (left_val, right_val) => {

@bluss
Copy link
Member

bluss commented Nov 11, 2015

Oh that's a good point 😄.

@alexcrichton
Copy link
Member

Thanks! Could you also add a test for this?

@ollie27
Copy link
Member Author

ollie27 commented Nov 11, 2015

I've added a test, I hope it's in the right place.

@brson
Copy link
Contributor

brson commented Nov 11, 2015

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned brson Nov 11, 2015
@alexcrichton
Copy link
Member

@bors: r+ 780581e

@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 12, 2015
bors added a commit that referenced this pull request Nov 12, 2015
`format_args!` doesn't support none Sized types so we should just pass it the references to `left_val` and `right_val`.

The following works:
```rust
assert!([1, 2, 3][..] == vec![1, 2, 3][..])
```
So I would expect this to as well:
```rust
assert_eq!([1, 2, 3][..], vec![1, 2, 3][..])
```
But it fails with "error: the trait `core::marker::Sized` is not implemented for the type `[_]` [E0277]"
I don't know if this change will have any nasty side effects I don't understand.
@bors
Copy link
Contributor

bors commented Nov 12, 2015

⌛ Testing commit 780581e with merge 0bd7084...

@bors bors merged commit 780581e into rust-lang:master Nov 12, 2015
@ollie27 ollie27 deleted the assert_eq_unsized branch May 2, 2016 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants