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 to_usize #12

Closed
wants to merge 1 commit into from
Closed

Add to_usize #12

wants to merge 1 commit into from

Conversation

matklad
Copy link
Member

@matklad matklad commented Mar 12, 2020

The main benefit of .to_usize versus .into() is that the type is
fixed for the purposes of type-inference. This is handly if you want
to do "hello, world"[..ts.to_usize()].

To clarify, the only reason why we might want this function is because "hello, world"[..ts] doesn't work, because of perma-unstable indexing trait in std :(

The main benefit of `.to_usize` versus `.into()` is that the type is
fixed for the purposes of type-inference. This is handly if you want
to do `"hello, world"[..ts.to_usize()]`.
@CAD97
Copy link
Collaborator

CAD97 commented Mar 13, 2020

I think the string[..size] case is probably better served by a string[TextRange::to(size)] (or maybe TextRange::from(..size)) than the ".ix()" method I had previously (roughly, "turn this into a standard indexing type").

Rowan doesn't use to_usize() at all. rust-analyzer does, so let's walk through the uses:

Conclusion: we definitely want TextRange::from(size), TextSize::one(), and a solution for replace_range. Otherwise, a nudge that to_usize is not an idiomatic code path (by not having a dedicated function) seems reasonable enough.

@CAD97
Copy link
Collaborator

CAD97 commented Mar 13, 2020

Notes from rust-lang/rust-analyzer#3570:

  • String::with_capacity(size.to_usize()) is a definite use of .to_usize(), but is equally served by .into()
  • The utf8_to_utf16_col version that uses usize more where it should almost certainly needs a type-hinted .to_usize() to be written the way that I did.

bors bot added a commit to rust-lang/rust-analyzer that referenced this pull request Mar 13, 2020
Merge #3570

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
3570: Remove some TextUnit->usize escapees r=matklad a=CAD97

As spotted during [a review of all uses of `text_unit::TextUnit::to_usize`](rust-analyzer/text-size#12 (comment)). Legitimate uses do remain.

Co-authored-by: CAD97 <cad97@cad97.com>
@matklad
Copy link
Member Author

matklad commented Mar 13, 2020

Hm, given the amount of false-positives in rust-analyzer, I think there's a clear benefit in not providing to_usize method :)

If only there was a clearly good name for TextRange::to(size)...

@matklad matklad closed this Mar 13, 2020
@matklad matklad deleted the to_usize branch March 20, 2020 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants