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

Fix std::convert::TryFrom doc #116594

Merged
merged 1 commit into from
Oct 15, 2023
Merged

Conversation

tae-soo-kim
Copy link
Contributor

Original text:

truncating the i64 to an i32 (essentially giving the i64’s value modulo i32::MAX)

This can't be true, because i32::MAX is an odd number. The correct value seems (i32::MAX + 1) * 2, but this is complicated and distracting, and I suggest removing the parentheses entirely.

@rustbot
Copy link
Collaborator

rustbot commented Oct 10, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @scottmcm (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 10, 2023
@scottmcm
Copy link
Member

Thanks for the PR! Agreed that modulo i32::MAX just doesn't make sense, so let's take this improvement as-is.
@bors r+ rollup

@tae-soo-kim, would you be interested in also doing a follow-up PR to improve the paragraph more broadly? The From trait recently got some new docs about what it should do (https://doc.rust-lang.org/nightly/std/convert/trait.From.html#when-to-implement-from), and that might inspire some similar for TryFrom. For example, successful TryFrom conversions should probably also be "value-preserving", so returning a modular result for integer-to-integer conversions would be a poor choice.

@bors
Copy link
Contributor

bors commented Oct 14, 2023

📌 Commit e15e9a6 has been approved by scottmcm

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2023
@tae-soo-kim
Copy link
Contributor Author

would you be interested in also doing a follow-up PR to improve the paragraph more broadly?

I guess no at this time. I'm still a beginner learning this language, worrying my understanding of it could be imprecise and misleading. But if you write something that I see obviously wrong, I'm happy to file a bug report.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2023
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#116594 (Fix `std::convert::TryFrom` doc)
 - rust-lang#116741 (Document `string_deref_patterns` feature)
 - rust-lang#116748 (Fix a spot I wrote the wrong word)
 - rust-lang#116753 (add 'Onur Özkan' to .mailmap)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e86e6b4 into rust-lang:master Oct 15, 2023
11 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Oct 15, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2023
Rollup merge of rust-lang#116594 - tae-soo-kim:convert-tryfrom-doc, r=scottmcm

Fix `std::convert::TryFrom` doc

Original text:

> truncating the [i64](https://doc.rust-lang.org/std/primitive.i64.html) to an [i32](https://doc.rust-lang.org/std/primitive.i32.html) (essentially giving the [i64](https://doc.rust-lang.org/std/primitive.i64.html)’s value modulo [i32::MAX](https://doc.rust-lang.org/std/primitive.i32.html#associatedconstant.MAX))

This can't be true, because `i32::MAX` is an odd number. The correct value seems `(i32::MAX + 1) * 2`, but this is complicated and distracting, and I suggest removing the parentheses entirely.
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 17, 2023
54: Pull upstream master 2023 10 17 r=pietroalbini a=Veykril

* rust-lang/rust#116196
* rust-lang/rust#116824
* rust-lang/rust#116822
* rust-lang/rust#116477
* rust-lang/rust#116826
* rust-lang/rust#116820
  * rust-lang/rust#116811
  * rust-lang/rust#116808
  * rust-lang/rust#116805
  * rust-lang/rust#116800
  * rust-lang/rust#116798
  * rust-lang/rust#116754
* rust-lang/rust#114370
* rust-lang/rust#116804
  * rust-lang/rust#116802
  * rust-lang/rust#116790
  * rust-lang/rust#116786
  * rust-lang/rust#116709
  * rust-lang/rust#116430
  * rust-lang/rust#116257
  * rust-lang/rust#114157
* rust-lang/rust#116731
* rust-lang/rust#116550
* rust-lang/rust#114330
* rust-lang/rust#116724
* rust-lang/rust#116782
  * rust-lang/rust#116776
  * rust-lang/rust#115955
  * rust-lang/rust#115196
* rust-lang/rust#116775
* rust-lang/rust#114589
* rust-lang/rust#113747
* rust-lang/rust#116772
  * rust-lang/rust#116771
  * rust-lang/rust#116760
  * rust-lang/rust#116755
  * rust-lang/rust#116732
  * rust-lang/rust#116522
  * rust-lang/rust#116341
  * rust-lang/rust#116172
* rust-lang/rust#110604
* rust-lang/rust#110729
* rust-lang/rust#116527
* rust-lang/rust#116688
* rust-lang/rust#116757
  * rust-lang/rust#116753
  * rust-lang/rust#116748
  * rust-lang/rust#116741
  * rust-lang/rust#116594
* rust-lang/rust#116691
* rust-lang/rust#116643
* rust-lang/rust#116683
* rust-lang/rust#116635
* rust-lang/rust#115515
* rust-lang/rust#116742
  * rust-lang/rust#116661
  * rust-lang/rust#116576
  * rust-lang/rust#116540
* rust-lang/rust#116352
* rust-lang/rust#116737
  * rust-lang/rust#116730
  * rust-lang/rust#116723
  * rust-lang/rust#116715
  * rust-lang/rust#116603
  * rust-lang/rust#116591
  * rust-lang/rust#115439
* rust-lang/rust#116264
* rust-lang/rust#116727
  * rust-lang/rust#116704
  * rust-lang/rust#116696
  * rust-lang/rust#116695
  * rust-lang/rust#116644
  * rust-lang/rust#116630
* rust-lang/rust#116728
  * rust-lang/rust#116689
  * rust-lang/rust#116679
  * rust-lang/rust#116618
  * rust-lang/rust#116577
  * rust-lang/rust#115653
* rust-lang/rust#116702
* rust-lang/rust#116015
* rust-lang/rust#115822
* rust-lang/rust#116407
* rust-lang/rust#115719
* rust-lang/rust#115524
* rust-lang/rust#116705
* rust-lang/rust#116645
* rust-lang/rust#116233
* rust-lang/rust#115108
* rust-lang/rust#116670
* rust-lang/rust#116676
* rust-lang/rust#116666



Co-authored-by: Benoît du Garreau <[email protected]>
Co-authored-by: Colin Finck <[email protected]>
Co-authored-by: Ian Jackson <[email protected]>
Co-authored-by: Joshua Liebow-Feeser <[email protected]>
Co-authored-by: León Orell Valerian Liehr <[email protected]>
Co-authored-by: Trevor Gross <[email protected]>
Co-authored-by: Evan Merlock <[email protected]>
Co-authored-by: joboet <[email protected]>
Co-authored-by: Ralf Jung <[email protected]>
Co-authored-by: DaniPopes <[email protected]>
Co-authored-by: Mark Rousskov <[email protected]>
Co-authored-by: onur-ozkan <[email protected]>
Co-authored-by: Nicholas Nethercote <[email protected]>
Co-authored-by: The 8472 <[email protected]>
Co-authored-by: Samuel Thibault <[email protected]>
Co-authored-by: reez12g <[email protected]>
Co-authored-by: Jakub Beránek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants