Skip to content

Make LenType opt-in #569

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Make LenType opt-in #569

wants to merge 3 commits into from

Conversation

sosthene-nitrokey
Copy link
Contributor

@sosthene-nitrokey sosthene-nitrokey commented Apr 29, 2025 •
edited
Loading

See #568

This makes LenType all default to usize and prepares for a 0.9.1 release (assuming you chose to yank 0.9.0, as I suggested)

Regarding SortedLinkedList:

Before https://github.com/rust-embedded/heapless/pull/553/files it was already using and Idx type for the same optimization. The PR only added the DefaultLenType, but all previous code needed to make the type explicit. I think it might be fine to leave it there and not default to usize (but at the cost of having everyone needing to re-order the generics in their types).

@sosthene-nitrokey
Copy link
Contributor Author

We can also add back some new type aliases that use the DefaultLenType mechanisms, next to the main Vec and String types. But I didn't want this to be blocked by bikeshedding on the name of those.

@sosthene-nitrokey
Copy link
Contributor Author

sosthene-nitrokey commented Apr 29, 2025 •
edited
Loading

Wait, maybe a release should not be made with just that. 0.9.0 was missing multiple wanted breaking changes: #494

@sosthene-nitrokey
Copy link
Contributor Author

To consider merging before this: 

@sosthene-nitrokey sosthene-nitrokey force-pushed the default-len-type branch 2 times, most recently from 2f6e693 to 840c7a7 Compare April 29, 2025 15:08
@zeenix
Copy link
Contributor

zeenix commented Apr 29, 2025

Wait, maybe a release should not be made with just that. 0.9.0 was missing multiple wanted breaking changes: #494

I think the idea was that 0.9.0 has waited already very long and any more breaking changes should go into the next breaking release (perhaps 1.0).

reitermarkus
reitermarkus reviewed Apr 29, 2025
View reviewed changes
Cargo.toml Outdated
@@ -12,7 +12,7 @@ keywords = ["static", "no-heap"]
license = "MIT OR Apache-2.0"
name = "heapless"
repository = "https://github.com/rust-embedded/heapless"
version = "0.9.0"
version = "0.9.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this into a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved into #574

@sosthene-nitrokey sosthene-nitrokey mentioned this pull request Apr 29, 2025
Open
3 tasks
@sosthene-nitrokey sosthene-nitrokey changed the title Make LenType opt-in and prepare release 0.9.1 Make LenType opt-in Apr 29, 2025
@sosthene-nitrokey sosthene-nitrokey force-pushed the default-len-type branch 2 times, most recently from b947a5b to 923847d Compare April 29, 2025 18:30
@sosthene-nitrokey
Copy link
Contributor Author

sosthene-nitrokey commented Apr 29, 2025 •
edited
Loading

I think the idea was that 0.9.0 has waited already very long and any more breaking changes should go into the next breaking release (perhaps 1.0).

These breaking changes were not made part of the addition of the View types because my initial PRs were made with the goal of not including breaking changes to be included in a 0.8.1 release. These breaking changes are not very intrusive, and shouldn't need any code change for the majority of uses (and if it does it should be trivial). Adding them to the 0.9.0 release just makes the code simpler, both for heapless and for downstreams.

See #529 (comment)

@zeenix
Copy link
Contributor

zeenix commented Apr 29, 2025

I think the idea was that 0.9.0 has waited already very long and any more breaking changes should go into the next breaking release (perhaps 1.0).

These breaking changes were not made part of the addition of the View types because my initial PRs were made with the goal of not including breaking changes to be included in a 0.8.1 release.

Ah ok. Thanks for explaining. 👍

These breaking changes are not very intrusive, and shouldn't need any code change for the majority of uses (and if it does it should be trivial). Adding them to the 0.9.0 release just makes the code simpler, both for heapless and for downstreams.

Cool. I just hope that they're easy to review/merge and won't delay 0.9.1.

reitermarkus
reitermarkus reviewed Apr 29, 2025
View reviewed changes
CHANGELOG.md Outdated
@@ -18,6 +19,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
### Removed

- Removed invalid `bytes::Buf` implementation.
- Removed `DefaultLenType` struct
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Removed `DefaultLenType` struct
- Removed `DefaultLenType` struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

reitermarkus
reitermarkus reviewed Apr 29, 2025
View reviewed changes
src/vec/mod.rs Outdated
@@ -1806,7 +1805,7 @@ mod tests {
{
let v: Vec<Droppable, 2> = Vec::new();
let v: Box<Vec<Droppable, 2>> = Box::new(v);
let mut v: Box<VecView<Droppable, u8>> = v;
let mut v: Box<VecView<Droppable, usize>> = v;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now redundant, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reitermarkus reitermarkus reitermarkus left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sosthene-nitrokey @zeenix @reitermarkus