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

Tracking Issue for substr_range and related methods #126769

Open
3 of 5 tasks
wr7 opened this issue Jun 21, 2024 · 6 comments
Open
3 of 5 tasks

Tracking Issue for substr_range and related methods #126769

wr7 opened this issue Jun 21, 2024 · 6 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@wr7
Copy link
Contributor

wr7 commented Jun 21, 2024

Feature gate: #![feature(substr_range)]

This is a tracking issue for str::substr_range, slice::subslice_range, and slice::elem_offset as described in this ACP.

These methods can be used for error handling and to extend str::lines, str::split, slice::split, and other related methods.

Public API

impl str {
    fn substr_range(&self, substr: &str) -> Option<Range<usize>>;
}

impl<T> [T] {
    fn subslice_range(&self, subslice: &[T]) -> Option<Range<usize>>;
    fn element_offset(&self, elem: &T) -> Option<usize>;
}

Steps / History

Unresolved Questions

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@wr7 wr7 added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 21, 2024
@wr7 wr7 changed the title Tracking Issue for XXX Tracking Issue for substr_range and related methods Jun 21, 2024
@tgross35
Copy link
Contributor

Bikeshed: element_offset or item_offset seems better to me than abbreviating to elem, there aren't many truncated words in the slice method names. And e.g. std::simd uses rotate_elements_{left,right} and {Mask,Simd}Element (that is unstable API, but has been around for a while)

@wr7
Copy link
Contributor Author

wr7 commented Jun 22, 2024

Bikeshed: element_offset or item_offset seems better to me than abbreviating to elem, there aren't many truncated words in the slice method names. And e.g. std::simd uses rotate_elements_{left,right} and {Mask,Simd}Element (that is unstable API, but has been around for a while)

+1
I'm somewhat impartial to the naming, but I think naming it element_offset might make sense. It's much more descriptive than "item", and the current documentation for slices appears to use the term "element" much more than "item".

I originally thought that "element" seems kinda long, but it's actually shorter than "subslice", so I think it's fine.

The elem_offset naming for the method was from a t-libs-api meeting, so I think I'll wait to see if I get more feedback from the rust community on the name before renaming it.

@ericlagergren
Copy link

Should they be const?

@wr7
Copy link
Contributor Author

wr7 commented Jun 23, 2024

Should they be const?

Good question

I think that it would be great for them to be const, but a const implementation does not seem to be trivial in current-day rust.

I can see two main ways of implementing something similar to the proposed methods:

  1. (Current implementation) Cast the pointers into usizes and then use wrapping_sub and wrapping_add on them. The issue with this is that you cannot cast pointers into usizes in const contexts. I don't see this changing too soon in the future either.
  2. Using pointer::byte_offset_from. This is const, and this would work most of the time, but in cases where this method should return None, pointer::byte_offset_from would invoke undefined behavior, and there is no obvious way to get around this.

Fortunately, the most common use cases aren't currently const either. Methods like str::split return iterators which currently cannot be used in const contexts.

Making these methods const is a non-breaking change, so I personally think that we should move forwards with non-const implementations and file a separate issue in the future if constness is desired enough.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 24, 2024
Add elem_offset and related methods

Implementation of rust-lang#126769
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 24, 2024
Rollup merge of rust-lang#126770 - wr7:master, r=Amanieu

Add elem_offset and related methods

Implementation of rust-lang#126769
@jongiddy
Copy link
Contributor

jongiddy commented Nov 3, 2024

The documentation for subslice_range says:

Returns None if subslice does not point within the slice or if it points between elements.

My initial impression of the second part of this statement was that an empty slice that was within the slice would return None.

Looking at the code, the meaning appears to be that a slice that points to a byte inside a single element returns None. That is the opposite of pointing between elements. Can this be rewritten as:

Returns None if subslice does not point to the start of an element within the slice or to the end of the slice.

The same comment applies to elem_offset but it is more restricted:

Returns None if element does not point to the start of an element within the slice.

@wr7
Copy link
Contributor Author

wr7 commented Nov 9, 2024

The documentation for subslice_range says:

Returns None if subslice does not point within the slice or if it points between elements.

My initial impression of the second part of this statement was that an empty slice that was within the slice would return None.

Looking at the code, the meaning appears to be that a slice that points to a byte inside a single element returns None. That is the opposite of pointing between elements. Can this be rewritten as:

Returns None if subslice does not point to the start of an element within the slice or to the end of the slice.

The same comment applies to elem_offset but it is more restricted:

Returns None if element does not point to the start of an element within the slice.

I've created a PR (#132830) that includes the documentation changes that you suggested (with some minor modifications) and implements the renaming suggested by @tgross35.

jhpratt added a commit to jhpratt/rust that referenced this issue Dec 20, 2024
…tgross35

Rename `elem_offset` to `element_offset`

Tracking issue: rust-lang#126769

Renames `slice::elem_offset` to `slice::element_offset` and improves the documentation of it and its related methods.

The current documentation can be misinterpreted (as explained [here](rust-lang#126769 (comment))).
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 20, 2024
Rollup merge of rust-lang#132830 - wr7:substr_range_documentation, r=tgross35

Rename `elem_offset` to `element_offset`

Tracking issue: rust-lang#126769

Renames `slice::elem_offset` to `slice::element_offset` and improves the documentation of it and its related methods.

The current documentation can be misinterpreted (as explained [here](rust-lang#126769 (comment))).
RalfJung pushed a commit to RalfJung/miri that referenced this issue Dec 20, 2024
Rename `elem_offset` to `element_offset`

Tracking issue: #126769

Renames `slice::elem_offset` to `slice::element_offset` and improves the documentation of it and its related methods.

The current documentation can be misinterpreted (as explained [here](rust-lang/rust#126769 (comment))).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants