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

restrict the state argument to iterate to Int64 for some ranges #57593

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Mar 1, 2025

This seems like it should eliminate a great number of invalidations when defining a new subtype of Integer, according to SnoopCompile.jl and friends.

This is correct because:

  • the state starts from zero
  • iterate doesn't ever skip any state, that is, each successive state is visited during iteration
  • it's not physically possible to iterate all the way from zero to typemax(Int64)

This seems like it should eliminate a great number of invalidations
when defining a new subtype of `Integer`, according to SnoopCompile.jl
and friends.

This is correct because:
* the state starts from zero
* `iterate` doesn't ever skip any state, that is, each successive state
  is visited during iteration
* it's not physically possible to iterate all the way from zero to
  `typemax(Int64)`
@nsajko nsajko added ranges Everything AbstractRange invalidations labels Mar 1, 2025
@nsajko
Copy link
Contributor Author

nsajko commented Mar 1, 2025

  • If there's some code that violates the second assumption, that would seem like abuse of iterate and I'll fix it by introducing or using some other function instead of iterate for that use.
  • I guess this could result in a performance regression for 32-bit i686? Would that matter?

@nsajko
Copy link
Contributor Author

nsajko commented Mar 1, 2025

blocked by #57261

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalidations ranges Everything AbstractRange
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant