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

remove LengthAtMost32 on AsRef/Borrow impl for array #74026

Closed
wants to merge 1 commit into from
Closed

remove LengthAtMost32 on AsRef/Borrow impl for array #74026

wants to merge 1 commit into from

Conversation

burrbull
Copy link
Contributor

@burrbull burrbull commented Jul 4, 2020

Since I did not receive a response to #71373 I tried to do PR.

All tests are passed on my PC. Is there any reason not to do this?

Thanks.

@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 @withoutboats (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 rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2020
@crlf0710
Copy link
Member

crlf0710 commented Jul 4, 2020

When these bounds are removed, the resulting impl surface will not be expressible in current stable Rust. So we have to keep these bounds until const generics is stablized...

@withoutboats withoutboats added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jul 4, 2020
@withoutboats
Copy link
Contributor

When these bounds are removed, the resulting impl surface will not be expressible in current stable Rust. So we have to keep these bounds until const generics is stablized...

We don't. By removing these bounds, we would be committing that std will always be able to implement traits for all array types. Since that's always been a completely consensus goal for everyone involved in Rust, and we have no reason to believe we won't eventually stabilize const generics at least powerful enough to implement traits for array types, I think we should remove this bound and provide this functionality to people sooner, rather than later.

The decision to introduce this bound was always unnecessary conservatism.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jul 4, 2020

Team member @withoutboats 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 4, 2020
@withoutboats
Copy link
Contributor

withoutboats commented Jul 4, 2020

@burrbull if we're going to do this, we should remove LengthAtMost32 entirely and change the implementation for all traits for arrays, not only asref/borrow.

@joshtriplett
Copy link
Member

I agree: we're just committing that the standard library will implement such traits, which does not inherently commit us to any particular design for stable const generics outside the standard library; that just means we must provide a language facility the standard library can use for this purpose.

@tesuji
Copy link
Contributor

tesuji commented Jul 5, 2020

Does this mean that now we could implement Copy, Hash, etc for all arrays?

@lcnr
Copy link
Contributor

lcnr commented Jul 5, 2020

One thing which concerns me is Default for arrays, which afaik we can't implement using const generics rn
as the current Default impl does not have a T: Default bound for N == 0.

So I fear that we will end up with Default being the only trait which is not implemented for all arrays.

@kpp kpp mentioned this pull request Jul 5, 2020
@burrbull
Copy link
Contributor Author

burrbull commented Jul 5, 2020

Closed in favour of 74060.

@burrbull burrbull closed this Jul 5, 2020
@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 5, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 26, 2020
Remove trait LengthAtMost32

This is a continuation of rust-lang#74026 preserving the original burrbull's commit.

I talked to @burrbull, he suggested me to finish his PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants