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] Span slicing with negative step #3650

Open
wants to merge 4 commits into
base: nightly
Choose a base branch
from

Conversation

msaelices
Copy link
Contributor

Fixes #3649

  1> s = String("01234")  
(String) s = "01234"
  2> for i in s.as_bytes_span()[::-1]: 
  3.   print(chr(int(i[]))) 
  4.    
4
3
2
1
0
  5>  

@msaelices msaelices changed the base branch from main to nightly October 12, 2024 13:41
@msaelices msaelices requested a review from a team as a code owner October 12, 2024 13:41
dest_ptr[i] = self._data[start]
start -= step
i += 1
return Span[T, lifetime](unsafe_ptr=dest_ptr, len=new_len)
Copy link
Contributor Author

@msaelices msaelices Oct 12, 2024

Choose a reason for hiding this comment

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

What's the best way to ensure the buffer allocated memory is destroyed when the span is deleted?

Copy link
Contributor

@martinvuyk martinvuyk Oct 12, 2024

Choose a reason for hiding this comment

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

Hmmm, not sure since I don't really understand lifetimes yet. But I think building a var items = List[T](unsafe_pointer=dest_ptr, size=new_len, capacity=new_len) and returning Span[T, lifetime](items^) will work?

Either way I think this should probably return an iterator in the future (once we have them), it doesn't make much sense to allocate for this since it is a read only view on the data.

msaelices added a commit to msaelices/aoc2023 that referenced this pull request Oct 12, 2024
This needs for this PR to be merged into the nightly release, as the negative steps are not working in Mojo: modularml/mojo#3650

Signed-off-by: Manuel Saelices <[email protected]>
@soraros
Copy link
Contributor

soraros commented Oct 12, 2024

  • Span suppose to be a non-owning view, I don't think this is the right fix.
  • Actually, I'm not even sure a span with only a length is capable of representing slicing with negative step at all.

@msaelices
Copy link
Contributor Author

  • Span suppose to be a non-owning view, I don't think this is the right fix.
  • Actually, I'm not even sure a span with only a length is capable of representing slicing with negative step at all.

Yes, it's a little bit weird. However, in Python you can slice a slice, like this:

>>> l = list(range(10))[2:5][::-1]
>>> l
[4, 3, 2]
>>> 

How Mojo could support this kind of operations without the support of negative steps?

@bgreni
Copy link
Contributor

bgreni commented Oct 13, 2024

  • Actually, I'm not even sure a span with only a length is capable of representing slicing with negative step at all.

I made a change previously to kinda hack it by giving Span a Slice member here #2950, but apparently it caused issues and was reverted internally? It feels like something we should be able to do though.

@msaelices
Copy link
Contributor Author

  • Actually, I'm not even sure a span with only a length is capable of representing slicing with negative step at all.

I made a change previously to kinda hack it by giving Span a Slice member here #2950, but apparently it caused issues and was reverted internally? It feels like something we should be able to do though.

That is amazing! I wonder why this was reverted. To me it's a must to have slicing matching up the Python ones.

@soraros
Copy link
Contributor

soraros commented Oct 13, 2024

#2950 essentially demonstrates the points I made above: Span itself is indeed incapable of representing stride. I believe it's still desirable for Span to remain a simple pointer-length pair. Perhaps we could introduce a StridedSpan?

@martinvuyk
Copy link
Contributor

What do you think of this proposal #3653 ?

@bgreni
Copy link
Contributor

bgreni commented Oct 14, 2024

#2950 essentially demonstrates the points I made above: Span itself is indeed incapable of representing stride. I believe it's still desirable for Span to remain a simple pointer-length pair. Perhaps we could introduce a StridedSpan?

That's reasonable, I wish we had some context for why that change didn't work internally though.

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.

[BUG] Span reversed slicing are not giving the right values
4 participants