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

[stdlib] Make Slice step Optional #3160

Closed
wants to merge 1 commit into from

Conversation

bgreni
Copy link
Contributor

@bgreni bgreni commented Jul 1, 2024

Follow up from #3064 (comment)

Make step member of Slice Optional in order to better match the behaviour/semantics of the Python slice.

@bgreni bgreni requested a review from a team as a code owner July 1, 2024 18:52
@bgreni bgreni changed the title Make Slice step OptionalReg [stdlib] Make Slice step OptionalReg Jul 1, 2024
Copy link
Collaborator

@JoeLoser JoeLoser left a comment

Choose a reason for hiding this comment

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

@ConnorGray or @rparolin - do you mind checking the perf impact of this, please?

@bgreni bgreni force-pushed the slice-step-optional branch 2 times, most recently from 3250ae9 to 0d9348d Compare July 7, 2024 15:55
@JoeLoser
Copy link
Collaborator

JoeLoser commented Jul 8, 2024

Let's hold off on this for a bit since @lattner has a PR up internally to change Slice to use Optional for its data members.

@JoeLoser
Copy link
Collaborator

Slice uses Optional now, so can you try making step an Optional?

@bgreni bgreni force-pushed the slice-step-optional branch from 0d9348d to 6ee674f Compare July 11, 2024 02:16
@bgreni bgreni changed the title [stdlib] Make Slice step OptionalReg [stdlib] Make Slice step Optional Jul 11, 2024
@bgreni bgreni force-pushed the slice-step-optional branch 2 times, most recently from cb01789 to 1c574ae Compare July 17, 2024 20:10
@bgreni bgreni force-pushed the slice-step-optional branch 2 times, most recently from f55b116 to 63b294f Compare August 4, 2024 20:15
@bgreni bgreni force-pushed the slice-step-optional branch 3 times, most recently from 6fd634b to f9fcaeb Compare August 25, 2024 00:36
@bgreni bgreni force-pushed the slice-step-optional branch 2 times, most recently from 7d24a10 to 651e5fe Compare September 10, 2024 20:05
@bgreni bgreni force-pushed the slice-step-optional branch 2 times, most recently from 208dbbd to 9399dc4 Compare September 20, 2024 16:39
@bgreni bgreni force-pushed the slice-step-optional branch from 9399dc4 to e5887da Compare October 8, 2024 21:31
@jjvraw
Copy link
Contributor

jjvraw commented Oct 9, 2024

Hey @bgreni, what's the benefit of making step Optional?

Signed-off-by: Brian Grenier <[email protected]>
@bgreni bgreni force-pushed the slice-step-optional branch from e5887da to bd5047a Compare October 10, 2024 15:50
@bgreni
Copy link
Contributor Author

bgreni commented Oct 10, 2024

Hey @bgreni, what's the benefit of making step Optional?

@jjvraw Hello, it's simply to match the Python implementation.

@JoeLoser
Copy link
Collaborator

!sync

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label Oct 13, 2024
@modularbot
Copy link
Collaborator

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

@modularbot modularbot added merged-internally Indicates that this pull request has been merged internally merged-externally Merged externally in public mojo repo labels Oct 15, 2024
@modularbot
Copy link
Collaborator

Landed in 630f621! Thank you for your contribution 🎉

modularbot pushed a commit that referenced this pull request Oct 16, 2024
Follow up from
#3064 (comment)

Make `step` member of `Slice` `Optional` in order to better match the
behaviour/semantics of the Python slice.

Co-authored-by: bgreni <[email protected]>
Closes #3160
MODULAR_ORIG_COMMIT_REV_ID: dcfe50395856e038b64deb6d88e8d68a69edeae4
@modularbot modularbot closed this Oct 16, 2024
modularbot pushed a commit that referenced this pull request Dec 17, 2024
Follow up from
#3064 (comment)

Make `step` member of `Slice` `Optional` in order to better match the
behaviour/semantics of the Python slice.

Co-authored-by: bgreni <[email protected]>
Closes #3160
MODULAR_ORIG_COMMIT_REV_ID: dcfe50395856e038b64deb6d88e8d68a69edeae4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants