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

Clarify undefined behaviour in binary heap, btree and hashset docs #87537

Merged
merged 1 commit into from
Oct 22, 2021

Conversation

Wilfred
Copy link
Contributor

@Wilfred Wilfred commented Jul 28, 2021

Previously, it wasn't clear whether "This could include" was referring to logic errors, or undefined behaviour. Tweak wording to clarify this sentence does not relate to UB.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2021
@SkiFire13
Copy link
Contributor

I don't think this makes it more clear, quite the opposite, at first glance I thought the "behaviour" you added was referring to the "undefined behaviour".

You should also change the docs for HashMap, HashSet, BTreeMap and BTreeSet since they all include that same sentence

@kpreid
Copy link
Contributor

kpreid commented Jul 28, 2021

I don't think this makes it more clear, quite the opposite, at first glance I thought the "behaviour" you added was referring to the "undefined behaviour".

How about wording like this? “The behavior resulting from such a logic error is not specified (it could include panics, incorrect results, aborts, memory leaks, or non-termination) but will not be undefined behavior.”

(I think it would also be good to link “undefined behavior” to some page explaining what undefined behavior is, since this might be the first time someone has encountered the concept, and it might not be obvious that “undefined behavior” is a specific formal term.)

@Mark-Simulacrum
Copy link
Member

Agreed that this seems like a good clarification.

It may make sense to extract the shared language somewhere in the root of the library docs as a general statement we can link to in terms of what logic errors can do; that way we can avoid updating N places each time and potentially expand a little on the paragraph. std's top level doc comment seems like a reasonable place to add that section, I guess.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 1, 2021
@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 20, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@Wilfred can you please address the comments from the reviewer? Thank you.

@Wilfred Wilfred changed the title Clarify undefined behaviour in BinaryHeap docs Clarify undefined behaviour in binary heap, btree and hashset docs Sep 7, 2021
@Wilfred
Copy link
Contributor Author

Wilfred commented Sep 7, 2021

OK, I've updated the docs in all four places using the wording suggested by @kpreid :). The patch isn't identical in all those files as they've used different line lengths, so I followed the convention in each file.

@the8472 the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 8, 2021
@bors
Copy link
Contributor

bors commented Sep 26, 2021

☔ The latest upstream changes (presumably #89262) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 19, 2021
Previously, it wasn't clear whether "This could include" was referring
to logic errors, or undefined behaviour. Tweak wording to clarify this
sentence does not relate to UB.
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

Rebased and approved, thanks!

@bors
Copy link
Contributor

bors commented Oct 21, 2021

📌 Commit 04c1ec5 has been approved by Mark-Simulacrum

@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 21, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 21, 2021
…rk-Simulacrum

Clarify undefined behaviour in binary heap, btree and hashset docs

Previously, it wasn't clear whether "This could include" was referring to logic errors, or undefined behaviour. Tweak wording to clarify this sentence does not relate to UB.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 21, 2021
…rk-Simulacrum

Clarify undefined behaviour in binary heap, btree and hashset docs

Previously, it wasn't clear whether "This could include" was referring to logic errors, or undefined behaviour. Tweak wording to clarify this sentence does not relate to UB.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 22, 2021
…rk-Simulacrum

Clarify undefined behaviour in binary heap, btree and hashset docs

Previously, it wasn't clear whether "This could include" was referring to logic errors, or undefined behaviour. Tweak wording to clarify this sentence does not relate to UB.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 22, 2021
…rk-Simulacrum

Clarify undefined behaviour in binary heap, btree and hashset docs

Previously, it wasn't clear whether "This could include" was referring to logic errors, or undefined behaviour. Tweak wording to clarify this sentence does not relate to UB.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 22, 2021
Rollup of 14 pull requests

Successful merges:

 - rust-lang#87537 (Clarify undefined behaviour in binary heap, btree and hashset docs)
 - rust-lang#88624 (Stabilize feature `saturating_div` for rust 1.58.0)
 - rust-lang#89257 (Give better error for `macro_rules name`)
 - rust-lang#89665 (Ensure that pushing empty path works as before on verbatim paths)
 - rust-lang#89895 (Don't mark for loop iter expression as desugared)
 - rust-lang#89922 (Update E0637 description to mention `&` w/o an explicit lifetime name)
 - rust-lang#89944 (Change `Duration::[try_]from_secs_{f32, f64}` underflow error)
 - rust-lang#89991 (rustc_ast: Turn `MutVisitor::token_visiting_enabled` into a constant)
 - rust-lang#90028 (Reject closures in patterns)
 - rust-lang#90069 (Fix const qualification when executed after promotion)
 - rust-lang#90078 (Add a regression test for issue-83479)
 - rust-lang#90114 (Add some tests for const_generics_defaults)
 - rust-lang#90115 (Add test for issue rust-lang#78561)
 - rust-lang#90129 (triagebot: Treat `I-*nominated` like `I-nominated`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d91e9b7 into rust-lang:master Oct 22, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 22, 2021
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.

10 participants