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

sys: use Option's unwrap_unchecked() #81527

Closed
wants to merge 1 commit into from

Conversation

ojeda
Copy link
Contributor

@ojeda ojeda commented Jan 29, 2021

Now that #81383 is available, start using it.

Now that rust-lang#81383 is available,
start using it.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
@rust-highfive
Copy link
Collaborator

r? @sfackler

(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 Jan 29, 2021
@ojeda
Copy link
Contributor Author

ojeda commented Jan 29, 2021

I just realized wasm32::unreachable() is intended to always generate the unreachable instruction, which in the end is like a trap, rather than being UB like the unreachable() intrinsic (the name is a bit unfortunate...).

Perhaps this should call abort() instead to be clear and match the comment.

@ojeda ojeda closed this Jan 29, 2021
@ojeda ojeda deleted the sys-use-unwrap_unchecked branch January 29, 2021 18:53
ojeda added a commit to ojeda/rust that referenced this pull request Jan 29, 2021
Rationale:

  - `abort()` lowers to `wasm32::unreachable()` anyway.
  - `abort()` isn't `unsafe`.
  - `abort()` matches the comment better.
  - `abort()` avoids confusion by future readers (e.g.
    rust-lang#81527): the naming of wasm's
    `unreachable' instruction is a bit unfortunate because it is not
    related to the `unreachable()` intrinsic (intended to trigger UB).

Codegen is likely to be different since `unreachable()` is `inline`
while `abort()` is `cold`. Since it doesn't look like we are expecting
here to trigger this case, the latter seems better anyway.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
jackh726 added a commit to jackh726/rust that referenced this pull request Feb 2, 2021
…32-unreachable, r=Mark-Simulacrum

sys: use `process::abort()` instead of `arch::wasm32::unreachable()`

Rationale:

  - `abort()` lowers to `wasm32::unreachable()` anyway.
  - `abort()` isn't `unsafe`.
  - `abort()` matches the comment better.
  - `abort()` avoids confusion by future readers (e.g. rust-lang#81527): the naming of wasm's `unreachable` instruction is a bit unfortunate because it is not related to the `unreachable()` intrinsic (intended to trigger UB).

Codegen is likely to be different since `unreachable()` is `inline` while `abort()` is `cold`. Since it doesn't look like we are expecting here to trigger this case, the latter seems better anyway.
jackh726 added a commit to jackh726/rust that referenced this pull request Feb 2, 2021
…32-unreachable, r=Mark-Simulacrum

sys: use `process::abort()` instead of `arch::wasm32::unreachable()`

Rationale:

  - `abort()` lowers to `wasm32::unreachable()` anyway.
  - `abort()` isn't `unsafe`.
  - `abort()` matches the comment better.
  - `abort()` avoids confusion by future readers (e.g. rust-lang#81527): the naming of wasm's `unreachable` instruction is a bit unfortunate because it is not related to the `unreachable()` intrinsic (intended to trigger UB).

Codegen is likely to be different since `unreachable()` is `inline` while `abort()` is `cold`. Since it doesn't look like we are expecting here to trigger this case, the latter seems better anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants