Skip to content

Conversation

Lokathor
Copy link
Contributor

@Lokathor Lokathor commented May 8, 2024

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
Copy link
Collaborator

rustbot commented May 8, 2024

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.

Copy link
Member

@thomcc thomcc left a comment

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.

@the8472
Copy link
Member

the8472 commented May 24, 2024

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
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
Copy link
Member

the8472 commented May 24, 2024

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.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@dtolnay
Copy link
Member

dtolnay commented May 27, 2024

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented May 27, 2024

📌 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
@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
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
Contributor Author

i thought it was handled by bors >.>

@RalfJung
Copy link
Member

RalfJung commented May 28, 2024 via email

@Lokathor
Copy link
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
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.