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

Generalize signature of checkbounds(A, I...) #56502

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

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Nov 8, 2024

This method checks if indices are within bounds, and throws an error if they aren't. Custom types such as Broadcasted, therefore, would only need to implement the bounds-checking logic, and may leave the error-throwing bit to Base. They may do this by specializing checkbounds(Bool, A::MyType, I...) instead of checkbounds(A, I...), and the fallback implementation of the latter would handle processing the result.

@jishnub jishnub added the arrays [a, r, r, a, y, s] label Nov 8, 2024
@mbauman
Copy link
Member

mbauman commented Nov 8, 2024

I'm not thrilled about this, because without this restriction checkbounds(Bool, 1) becomes ambiguous in its meaning. Do you want the erroring method for an "indexing" expression like Bool[1]? Or do you want the non-erroring method for 1[]?

The docs are currently explicitly all about arrays. It might be nice to generalize it to all generic indexables (and we do already have string methods), but I think we'd need a different method dispatch design to accommodate that.

@jishnub jishnub marked this pull request as draft November 8, 2024 16:28
@mbauman
Copy link
Member

mbauman commented Nov 8, 2024

I think it'd also be good to think about how this works in general with other indexable things. The name itself is quite array-ish — it implies that there are contiguous regions of valid indices. But indexing also works with sets of non-contiguous keys; like dicts and named tuples and such. Should folks expect this to work like an erroring haskey for them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants