Skip to content

Update Result docs to the new guarantees#124870

Closed
Lokathor wants to merge 9 commits into
rust-lang:masterfrom
Lokathor:update-result-docs
Closed

Update Result docs to the new guarantees#124870
Lokathor wants to merge 9 commits into
rust-lang:masterfrom
Lokathor:update-result-docs

Conversation

@Lokathor

@Lokathor Lokathor commented May 8, 2024

Copy link
Copy Markdown
Contributor

Note: This PR did get merged, even though Github claims it did not.

The Option docs already explain the guarantees given in RFC 3391, so all that we need is a paragraph saying that some Result type combinations will also qualify.

Part of #110503

@rustbot

rustbot commented May 8, 2024

Copy link
Copy Markdown
Collaborator

r? @m-ou-se

rustbot has assigned @m-ou-se.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 May 8, 2024
@rust-log-analyzer

This comment has been minimized.

Comment thread library/core/src/result.rs Outdated

@thomcc thomcc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine except for the unrelated changes.

Comment thread library/core/src/result.rs Outdated
@the8472

the8472 commented May 24, 2024

Copy link
Copy Markdown
Member

The option module page currently is the central place for NPO documentation. If Result gets those too then things become more scattered. Maybe the Option page should also link to the Result page.

@Lokathor

Copy link
Copy Markdown
Contributor Author

How exactly do you think that would look @the8472 ?

The current wording is intended to be a "one way" lookup. "if X is the case, you can count on Y too", but since the guaranteed effects are part of Option then I'm not sure how Option would refer to Result.

@the8472

the8472 commented May 24, 2024

Copy link
Copy Markdown
Member

Maybe

Under some conditions the above types T are also null pointer optimized when wrapped in a [Result#Representation]

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@dtolnay dtolnay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

@dtolnay

dtolnay commented May 27, 2024

Copy link
Copy Markdown
Member

@bors r+ rollup

@bors

bors commented May 27, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit 9b480da has been approved by dtolnay

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 May 27, 2024
@dtolnay dtolnay assigned dtolnay and unassigned m-ou-se May 27, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 27, 2024
…lnay

Update Result docs to the new guarantees

The `Option` docs already explain the guarantees given in [RFC 3391](https://github.com/rust-lang/rfcs/blob/master/text/3391-result_ffi_guarantees.md), so all that we need is a paragraph saying that some `Result` type combinations will also qualify.

Part of rust-lang#110503
@bors bors closed this in 22668e8 May 27, 2024
@Lokathor Lokathor deleted the update-result-docs branch May 28, 2024 08:13
@RalfJung

Copy link
Copy Markdown
Member

Why is this "closed", not "merged"? Something seems to have gone wrong?

(Also the commits would ideally have been squashed before landing this, no need to have all these 1-line-fixup commits perpetually in the git history...)

@Lokathor

Copy link
Copy Markdown
Contributor Author

i thought it was handled by bors >.>

@RalfJung

RalfJung commented May 28, 2024 via email

Copy link
Copy Markdown
Member

@Lokathor

Copy link
Copy Markdown
Contributor Author

https://github.com/rust-lang/rust/blob/master/library/core/src/result.rs#L231-L250

the PR's changes do seem to be in the master branch

@RalfJung

Copy link
Copy Markdown
Member

Yeah, 9b480da got merged. No idea why this is not reflected in the PR state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

relnotes Marks issues that should be documented in the release notes of the next release. 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.