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

add slice_at #18066

Closed
wants to merge 1 commit into from
Closed

add slice_at #18066

wants to merge 1 commit into from

Conversation

mahkoh
Copy link
Contributor

@mahkoh mahkoh commented Oct 15, 2014

No description provided.

@alexcrichton
Copy link
Member

This is modifying a trait in the prelude, and there is no rationale here other than "add slice_at". Could you expand the description, title, or commit message to explain the reasoning for adding a new method like this? This will also need some tests in order to be merged as well.

@Gankra
Copy link
Contributor

Gankra commented Oct 15, 2014

This doesn't strike me as well-motivated. It is hardly a burden to do this with existing methods, as the implementations show. We're trying to reduce the api surface area right now. If this is really desirable, I think it can be re-evaluated after 1.0.

@mahkoh
Copy link
Contributor Author

mahkoh commented Oct 15, 2014

@gankro: How can slice_to be fine if this is not? Are you suggesting removing slice_to? That one even has a replacement with the new slicing syntax while slice_at does not.

@alexcrichton: Can you point me to the rationale for adding slice_to/from/etc.? I'm sure very similar justifications apply here.

@Gankra
Copy link
Contributor

Gankra commented Oct 15, 2014

I assume those will be deprecated by the slicing syntax when it's stable. However I haven't been following stabalization of slices (which I think was discussed privately during the last work-week?)

CC @aturon

@aturon
Copy link
Member

aturon commented Oct 15, 2014

Yes, the slice methods are going to be deprecated once slice syntax is un-feature-gated. We've discussed providing separate non-failing slice methods, but I don't think we're going to do that.

I agree that this convenience doesn't seem so well-motivated. You could make a stronger argument for it by showing some data about how often slice is used with a sum to get the to argument in existing Rust code. My impression is: not terribly often. (In general, slice_from and slice_to seem much more common than slice.)

@mahkoh
Copy link
Contributor Author

mahkoh commented Oct 15, 2014

How slice_to, slice_from or the [..n] and [n..] syntax could ever be added but tailn gets deprecated and this function gets rejected as not motivated (even though they are much more orthogonal and often cannot be replaced without creating more noise in the code) is beyond me. Have it your way.

@mahkoh mahkoh closed this Oct 15, 2014
@aturon
Copy link
Member

aturon commented Oct 15, 2014

@mahkoh To be clear, no one was saying that this would be rejected out of hand, just that we wanted to discuss the motivation and impact. As I suggested, it'd be interesting to collect some data on how these methods are actually being used in practice.

I'm not sure I follow the argument about tailn, since it's equivalent to the [n..] notation.

The slice notation and functions we're using have a massive precedent in other languages, and the notation in particular was discussed in an RFC.

In general we encourage constructive feedback and discussion, and are quite willing to change our minds on things!

@Gankra
Copy link
Contributor

Gankra commented Oct 15, 2014

Also: unlike some other languages, a method not being included in the core definition does not prevent a library from implementing their favourite conveniences using an extension trait. If you find that you use foo[i, i+k] a lot, you can always add slice_at to slices locally with a custom SliceAt trait.

lnicola pushed a commit to lnicola/rust that referenced this pull request Sep 25, 2024
fix: Don't panic lsp writer thread on dropped receiver

Should reduce the noise a bit (rust-lang/rust-analyzer#18055). This removes the panic (and a follow up panic) when the server incorrectly shuts down, turning it into a proper late exit error.
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.

4 participants