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

ntuple: ensure eltype is always Int #55901

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

Conversation

barucden
Copy link
Contributor

Fixes #55790

@nsajko
Copy link
Contributor

nsajko commented Sep 27, 2024

I think it'd be slightly better to construct an Int, with an ::Int type assertion immediately afterwards, right at the beginning of the ntuple method.

  • I think this would be faster for BigInt input, as comparing Int with Int should be faster than with BigInt.
  • This would lead to more consistent error messages on the off chance someone passes in an Integer value whose Int constructor doesn't return Int.

@barucden barucden force-pushed the ntuple2 branch 2 times, most recently from f75394a to f77989b Compare September 28, 2024 12:56
@barucden
Copy link
Contributor Author

Makes sense, thank you.

base/ntuple.jl Outdated Show resolved Hide resolved
@barucden barucden force-pushed the ntuple2 branch 2 times, most recently from 884f48f to 1237bb8 Compare September 28, 2024 15:00
base/ntuple.jl Outdated Show resolved Hide resolved
@giordano giordano added the domain:collections Data structures holding multiple items, e.g. sets label Sep 29, 2024
@timholy
Copy link
Sponsor Member

timholy commented Sep 29, 2024

If there's any risk this could take a while to compile (e.g., f is big and @inline), then the pattern

function slow_to_compile(f::F, n::Int) where F
    # body
end
slow_to_compile(f::F, n::Integer) where F = slow_to_compile(f, convert(Int, n)::Int)

might be better: it ensures the "real work" function only gets compiled for Int.

@barucden
Copy link
Contributor Author

Got it! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:collections Data structures holding multiple items, e.g. sets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ntuple eltype depends on tuple length for UInt64
6 participants