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

Check type getitem #5549

Merged
merged 6 commits into from May 21, 2020
Merged

Check type getitem #5549

merged 6 commits into from May 21, 2020

Conversation

EPronovost
Copy link
Contributor

Check that calls to Type.__getitem__ are slices with no start or stop (e.g. intp[:]).
Raises a KeyError if not.
Previously, list(intp) would infinite loop.

Check that calls to `Type.__getitem__` are slices with no start or stop (e.g. `intp[:]`).
Raises a KeyError if not.
Previously, `list(intp)` would infinite loop.
@stuartarchibald
Copy link
Contributor

Thanks for the PR, much appreciated. Looks like there's a couple of test fails on CI at present, marked as "In progress".

@stuartarchibald stuartarchibald added this to the Numba 0.50RC milestone Apr 13, 2020
@EPronovost
Copy link
Contributor Author

Yeah there are several places that depend on the old behavior that I need to fix still. I wanted to see what you thought about it, or if there were any valid use cases of indexing by something other than an open slice that I'm not aware of.

@EPronovost
Copy link
Contributor Author

@stuartarchibald I think the CI errors are fixed.

@stuartarchibald
Copy link
Contributor

Thanks for fixing, queued for review.

@@ -257,7 +257,10 @@ def add_indices_to_kernel(self, kernel, index_names, ndim,
const_index_vars[dim], loc)
new_body.append(ir.Assign(getitemcall, getitemvar, loc))
# Get the type of this particular part of the index tuple.
one_index_typ = stmt_index_var_typ[dim]
if isinstance(stmt_index_var_typ, types.ConstSized):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, I found the affected test test_basic81 (numba.tests.test_stencils.TestManyStencils)

@sklam sklam added 4 - Waiting on author Waiting for author to respond to review 5 - Ready to merge Review and testing done, is ready to merge and removed 3 - Ready for Review 4 - Waiting on author Waiting for author to respond to review labels May 21, 2020
Copy link
Member

@sklam sklam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants