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

Signed indexed_iter? #60

Open
henryiii opened this issue Dec 16, 2024 · 2 comments
Open

Signed indexed_iter? #60

henryiii opened this issue Dec 16, 2024 · 2 comments

Comments

@henryiii
Copy link
Contributor

I often use signed values for computing locations on the grid, and one annoyance is that indexed_iter() forces usize. It would be nice to have some way to control the type of the values is produces. I think it would be invasive to add <T> to indexed_iter(), though, as Rust would start asking users to add the template param if you have a situation where the result could not be inferred. This is what I tried:

    pub fn indexed_iter<I>(&self) -> impl Iterator<Item = ((I, I), &T)> 
    where
    I: TryFrom<usize> + Div<I> + Rem<I> + Copy,
    I: From<<I as Div>::Output>,
    I: From<<I as Rem>::Output>,
    {
        let cols: I = self.cols.try_into().ok().expect("Type too small to hold cols");
        let rows: I = self.rows.try_into().ok().expect("Type too small to hold rows");
        self.data.iter().enumerate().map(move |(idx, i)| {
            let idx: I = idx.try_into().ok().unwrap();
            let position: (I, I) = match self.order {
                Order::RowMajor => ((idx / cols).into(), (idx % cols).into()),
                Order::ColumnMajor => ((idx % rows).into(), (idx / rows).into()),
            };
            (position, i)
        })
    }

Note that the bounds checking only happens once, rather than every time, as it would for a user converting to the type later. But call sites can't always infer I, requiring a type here sometimes where it wasn't required before. Would this make sense as indexed_iter_t, perhaps?

@becheran
Copy link
Owner

Hm... I am not so sure about adding another _t function. My goal with this library was to more or less mimic the features of rust's std vec type. Since as far as I know a vec can also only be accessed with an unsigned type, I don't really think it does make sense to add the featuer for grid. But maybe I am also wrong and std vec supports and inferred type by now?

See failing playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&code=fn+swap%28x%3A+i32%2C+y%3A+i32%29+-%3E+%28i32%2C+i32%29+%7B%0A++++return+%28y%2C+x%29%3B%0A%7D%0A%0Afn+main%28%29+%7B%0A++++let+v+%3D+vec%21%5B1%2C2%2C3%5D%3B%0A++++let+idx+%3A+i32+%3D+1%3B%0A++++println%21%28%22%7B%7D%22%2C+v%5Bidx%5D%29%3B%0A%7D%0A

@henryiii
Copy link
Contributor Author

henryiii commented Dec 18, 2024

For std::vec, you can do vec.iter().enumerate().map(|(i, v)| ...), which is conceptually the same as grid.indexed_iter.map(|ind, v| ...). In the docs for enumerate, it says "if you need a custom type, use zip(0..) instead". So I'm looking for the equivalent to vec.iter().zip(0..).map(|(v, i)| ...), which allows you to either pick a type (0i32.., for example), or will infer the type if possible. I don't see any equivalent currently for grid.

Not particularly happy with indexed_iter_t, but not sure how it could be more elegantly done.

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

No branches or pull requests

2 participants