Skip to content

DSTify BytesContainer #18463

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Nov 3, 2014
Merged

DSTify BytesContainer #18463

merged 2 commits into from
Nov 3, 2014

Conversation

japaric
Copy link
Member

@japaric japaric commented Oct 30, 2014

  • The BytesContainer::container_into_owned_bytes method has been removed
  • Methods that used to take BytesContainer implementors by value, now take them by reference. In particular, this breaks some uses of Path:
Path::new("foo")  // Still works
path.join(another_path) -> path.join(&another_path)

[breaking-change]


Re: container_into_owned_bytes, I've removed it because

  • Nothing in the whole repository uses it
  • Takes self by value, which is incompatible with unsized types (str)

The alternative to removing this method is to split BytesContainer into BytesContainer for Sized? and SizedBytesContainer: BytesContainer + Sized, where the second trait only contains the container_into_owned_bytes method. I tried this alternative in another branch and it works, but it seemed better not to create a new trait for an unused method.

Re: Breakage of Path methods

We could use the idea that @alexcrichton proposed in #18457 (add blanket impl BytesContainer for &T where T: BytesContainer + keep taking T: BytesContainer by value in Path methods) to avoid breaking any code.

r? @aturon
cc #16918

@rust-highfive
Copy link
Contributor

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@alexcrichton
Copy link
Member

I'm fine with removing container_into_owned_bytes, and I would personally vote for the impl BytesContainer for &T where T: BytesContainer route as it's probably wanted anyway (&String is certainly a container of bytes!). This is a pretty common trait where the impl may be worth it.

@aturon
Copy link
Member

aturon commented Oct 31, 2014

Agreed with @alexcrichton. Can you update with the proposed change for Path ergonomics, and then I'll re-r?

@japaric
Copy link
Member Author

japaric commented Oct 31, 2014

Addressed Path ergonomics and squashed.

@aturon re-r?

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Oct 31, 2014
@japaric
Copy link
Member Author

japaric commented Nov 2, 2014

Rebased. @alexcrichton or @aturon re-r?

@japaric
Copy link
Member Author

japaric commented Nov 2, 2014

@aturon / @alexcrichton Could you tell bors to retry?

The previous attempt failed because one of the bots was continuously failing an udp-related test. But, I think the bots are properly working now.

@aturon
Copy link
Member

aturon commented Nov 2, 2014

@japaric done!

bors added a commit that referenced this pull request Nov 3, 2014
- The `BytesContainer::container_into_owned_bytes` method has been removed

- Methods that used to take `BytesContainer` implementors by value, now take them by reference. In particular, this breaks some uses of Path:

``` rust
Path::new("foo")  // Still works
path.join(another_path) -> path.join(&another_path)
```

[breaking-change]

---

Re: `container_into_owned_bytes`, I've removed it because

- Nothing in the whole repository uses it
- Takes `self` by value, which is incompatible with unsized types (`str`)

The alternative to removing this method is to split `BytesContainer` into `BytesContainer for Sized?` and `SizedBytesContainer: BytesContainer + Sized`, where the second trait only contains the `container_into_owned_bytes` method. I tried this alternative [in another branch](https://github.com/japaric/rust/commits/bytes) and it works, but it seemed better not to create a new trait for an unused method.

Re: Breakage of `Path` methods

We could use the idea that @alexcrichton proposed in #18457 (add blanket `impl BytesContainer for &T where T: BytesContainer` + keep taking `T: BytesContainer` by value in `Path` methods) to avoid breaking any code.

r? @aturon 
cc #16918
@bors bors closed this Nov 3, 2014
@bors bors merged commit fe256f8 into rust-lang:master Nov 3, 2014
@japaric japaric deleted the bytes2 branch November 3, 2014 16:18
lnicola added a commit to lnicola/rust that referenced this pull request Nov 4, 2024
…r-msg-improvement

Improve error message for too new proc-macro server
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants