Skip to content

Conversation

stepancheg
Copy link
Contributor

Objective

Solution

  • Replace IndicesIter with either
  • Bevy already depends on either crate:

either = "1.13"

Testing

CI

@alice-i-cecile
Copy link
Member

No strong feelings here 🤔 It's nice to reduce this, but the dependency isn't 0 cost, even if it is used elsewhere in Bevy already.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change A-Math Fundamental domain-agnostic mathematical operations C-Dependencies A change to the crates that Bevy depends on S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 20, 2024
@Victoronz
Copy link
Contributor

Victoronz commented Oct 20, 2024

This change makes me wonder one thing:
Returning impl Iterator seems wasteful here, since all of the trait impls that the iterator type has are lost, yet the current actual type name isn't actually more complex to write directly.
If we replace the IndicesIter enum with either, we suddenly lose even more trait impls, when we could be returning a type alias instead (though this would regress error messages somewhat, since those expand type aliases).
F.e. this would allow calling len() (ExactSizeIterator), or rev() (DoubleEndedIterator) on the return type, and a lot more.
As Alice said, either isn't free, and we're not even exposing the benefits of using it here to the API consumer.

@alice-i-cecile alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Oct 20, 2024
@stepancheg
Copy link
Contributor Author

@Victoronz it was impl Iterator before this change.

If we need more precise trait, we can return impl ExactSizeIterator + FusedIterator. But practically it does not matter.

We can even do + DoubleEndedIterator if we need to, which we get for free with either, but don't have in current impl.

Anyway, if this PR is not a big deal. Definitely not worth a long discussion.

@stepancheg stepancheg closed this Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Dependencies A change to the crates that Bevy depends on S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants