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

Upgrade length/capacity-related functions to const-fn #194

Merged
merged 2 commits into from Oct 27, 2021

Conversation

bhgomes
Copy link
Contributor

@bhgomes bhgomes commented Aug 1, 2021

The following are upgraded:

  • Array::len
  • Array::is_empty
  • Array::capacity
  • Array::is_full
  • Array::remaining_capacity (added one to ArrayString because it didn't exist)
  • CapacityError::new

@c410-f3r
Copy link
Contributor

Arrayvec::new was const but then was renamed to Arrayvec::new_const because of bad codegen (now Arrayvec::new is just a regular method).
So, I guess an assembly demonstration of before and after will improve the changes of merging. Or maybe such thing won't be necessary, all depends on @bluss :)
IMHO, all methods should be const when possible and bad codegen should be a thing fixed by MIR or LLVM.

@bhgomes
Copy link
Contributor Author

bhgomes commented Aug 11, 2021

Not sure if this is an issue in this case since these are just accessing struct fields or performing integer arithmetic, but I could be mistaken.

@bluss
Copy link
Owner

bluss commented Aug 12, 2021

Thanks, yes certainly it would be expected that these methods can't run into the same problem that the constructor did, in particular in that case we changed the body/implementation to make it possible to make the functions const, and we don't do that here.

Thanks for making sure that arraystring is in sync with arrayvec

@lrazovic
Copy link

Will these changes be added to the next version? Since a lot of apps/crates depends on arrayvec it would be very useful to have const fn, so those methods can also be used in other calling functions, as explained in this clippy lint.

@bluss bluss merged commit a4690c9 into bluss:master Oct 27, 2021
@bluss
Copy link
Owner

bluss commented Oct 27, 2021

yes, should be a new version soon

@bluss bluss added this to the 0.7.2 milestone Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants