Skip to content

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)

@sosthene-nitrokey sosthene-nitrokey force-pushed the history-buffer-view-dedup branch from 2ae7e8b to 314fd85 Compare July 1, 2024 13:04
@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
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