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 HistoryBufferView on top of #486 #493

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

sosthene-nitrokey
Copy link
Contributor

@sosthene-nitrokey sosthene-nitrokey commented Jul 1, 2024

OldestOrdered was already part of the public API.

When merging we will need to add its removal to #494 so that there is only OldestOrederedView (which can be renamed to just OldestOrdered)

@Dirbaio
Copy link
Member

Dirbaio commented Jul 1, 2024

OldestOrdered was already part of the public API.

oof 🥲

maybe it's "less bad" to make OldestOrdered the "new" way already, and add a type alias at the root for compat that just ignores the const generic. This way in 0.9 we can just remove the type alias.

mod histbuf {
    pub struct OldestOrdered<'a, T> {
        inner: core::iter::Chain<core::slice::Iter<'a, T>, core::slice::Iter<'a, T>>,
    }
}

#[deprecated = "use `heapless::histbuf::OldestOrdered` instead."]
pub type OldestOrdered<'a, T, const N: usize> = OldestOrderedInner<'a, T>;

i'm not sure if there's any problem in "ignoring" N that way, it seems to work in quick testing.

@sosthene-nitrokey
Copy link
Contributor Author

i'm not sure if there's any problem in "ignoring" N that way, it seems to work in quick testing.

I did not know that was possible!

@sosthene-nitrokey
Copy link
Contributor Author

sosthene-nitrokey commented Jul 1, 2024

i'm not sure if there's any problem in "ignoring" N that way, it seems to work in quick testing.

I think I found a possible issue, there will be some cases where the compiler won't be able to infer N. For exemple the following compiles now, but wouldn't compile with the new alias approach.

fn testing<const N: usize>(iter: OldestOrdered<'_, u32, N> {}

fn main() {
   let a = HistoryBuffer::<u32, 3>::new();
   testing(a.oldest_ordered());
}

@Dirbaio
Copy link
Member

Dirbaio commented Jul 1, 2024

aw, that's a shame :(. Nice find. Let's do the PhantomData then.

@Dirbaio Dirbaio added this pull request to the merge queue Jul 1, 2024
Merged via the queue into rust-embedded:main with commit 7a321db Jul 1, 2024
22 checks passed
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.

2 participants