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

Implement Default for all array sizes #88744

Closed
shamatar opened this issue Sep 8, 2021 · 8 comments
Closed

Implement Default for all array sizes #88744

shamatar opened this issue Sep 8, 2021 · 8 comments
Labels
T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@shamatar
Copy link
Contributor

shamatar commented Sep 8, 2021

I've made a simple Godbolt for a demonstration of the approach.

In short, with array::map stabilized in 1.55 it's now possible to make an array [T; N] of default values of T by invoking [(); N].map(|_| T::default()). There is no difference in the emitted assembly for either O2 or O3 optimization levels in my short example for usize and user defined type (both ZST and not ZST), but obviously there is no guarantee that some more complex cases would not diverge.

I would expect that lifting a hard limitation of 32 on the size of the array would be handy in general.

@hellow554
Copy link
Contributor

There's an open issue for this... somewhere, I can't find it.
IIRC when implemented via const generics there was a regression with a zero sized array. Maybe somebody can link it here...

@shamatar
Copy link
Contributor Author

shamatar commented Sep 8, 2021

I've updated the Godbolt link to include creation of arrays of length 0 for both ZST and not ZST. It doesn't look like there is any difference as array::map already has a special case for N == 0 that is optimized out by the compiler in both cases.

@paolobarbolini
Copy link
Contributor

I've updated the Godbolt link to include creation of arrays of length 0 for both ZST and not ZST. It doesn't look like there is any difference as array::map already has a special case for N == 0 that is optimized out by the compiler in both cases.

The issue is that Default must also be implemented for zero sized arrays where T doesn't implement Default.

@cuviper
Copy link
Member

cuviper commented Sep 8, 2021

There's some discussion in #74060 (comment) and the references from there.

@shamatar
Copy link
Contributor Author

shamatar commented Sep 8, 2021

Ok, I see the point. It lead me to the question now posted in zullip, may be it would be a nice feature that may go along and parallel with specialization (and potentially much easier to implement)

@Gankra
Copy link
Contributor

Gankra commented May 26, 2022

Summary

Before we had const-generics we had some macros manually implementing Default for a few different array sizes, and we were too clever and special-cased [T; 0] to be unconditionally Default. As a result when const-generics were introduced, providing a const-generic impl of Default for arrays would have been a regression because it would introduce a T: Default bound to the empty array case. This regression may be acceptable but was rejected because:

  • It might break things (but seemingly no one actually did a crater run, citing the existence of non-public code)
  • Compiler devs believed they could/should "solve" the issue and so it would be needless churn to "break" it just to fix it again later

This all happened about 2 years ago, and there is currently no clear roadmap to resolving the issue either way.

History

@ryoqun
Copy link
Contributor

ryoqun commented Dec 19, 2022

There's an open issue for this... somewhere, I can't find it. IIRC when implemented via const generics there was a regression with a zero sized array. Maybe somebody can link it here...

@hellow554 I think #61415 is the correct issue.

@Gankra hey, it's nice summary. could you update to include a pointer to the issue?

@shamatar also, now it's directed to the correct issue. this one can be closed, maybe?

@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 26, 2022
@dtolnay
Copy link
Member

dtolnay commented Dec 26, 2022

Yes, I'll close this in favor of #61415.

@dtolnay dtolnay closed this as completed Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants