KEMBAR78
[mypyc] Add bytes index primitive by 97littleleaf11 · Pull Request #10950 · python/mypy · GitHub
Skip to content

Conversation

97littleleaf11
Copy link
Collaborator

@97littleleaf11 97littleleaf11 commented Aug 9, 2021

Description

Implements part of mypyc/mypyc#880.

  • Add two __getitem__ ops for bytes(int and short_int as index parameter)

Test Plan

Add several run tests and an irbuild test.

@97littleleaf11 97littleleaf11 marked this pull request as ready for review August 9, 2021 10:59
@97littleleaf11
Copy link
Collaborator Author

mistakely deleted an irbuilding test

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Looks mostly good, just a few small comments.

Can you also test this with a microbenchmark?

Copy link
Collaborator

@JukkaL JukkaL 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 updates! One more comment about tests, otherwise looks good.

b[4]
with assertRaises(IndexError, "index out of range"):
b[-5]
b2 = bytearray([175, 255, 128, 22])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use b2: bytes here as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This still needs to be updated.

@JukkaL
Copy link
Collaborator

JukkaL commented Aug 10, 2021

Also the Windows 32-bit build is failing.

@97littleleaf11 97littleleaf11 changed the title [mypyc] Add bytes index primitives [mypyc] Add bytes index primitive Aug 10, 2021
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

One remaining thing, otherwise ready to merge.

b[4]
with assertRaises(IndexError, "index out of range"):
b[-5]
b2 = bytearray([175, 255, 128, 22])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This still needs to be updated.

@97littleleaf11
Copy link
Collaborator Author

This test case is for bytearray setting, thus I prefer keeping b2 not annotated with bytes.

@JukkaL
Copy link
Collaborator

JukkaL commented Aug 11, 2021

Ah, fair enough. This is ready to merge then.

@JukkaL JukkaL merged commit ba7b50c into python:master Aug 11, 2021
@97littleleaf11 97littleleaf11 deleted the bytes-index branch August 11, 2021 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants