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.

Sign up for GitHub

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

Jump to bottom

#![deny(unsafe_op_in_unsafe_fn)] in sys/hermit #74979

Merged
merged 4 commits into from
Nov 6, 2020
Merged

#![deny(unsafe_op_in_unsafe_fn)] in sys/hermit #74979

merged 4 commits into from
Nov 6, 2020

Conversation

maekawatoshiki
Copy link
Contributor

@maekawatoshiki maekawatoshiki commented Jul 31, 2020 •
edited
Loading

Partial fix of #73904.

This encloses unsafe operations in unsafe fn in sys/hermit.
Some unsafe blocks are not well documented because some system-based functions lack documents.

@rust-highfive
Copy link
Collaborator

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 31, 2020
@LeSeulArtichaut LeSeulArtichaut mentioned this pull request Aug 2, 2020
Closed
22 tasks
@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Aug 19, 2020 •
edited
Loading

@joshtriplett this is ready for review reassigning

@Dylan-DPC-zz
Copy link

maybe

r? @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

I think it's probably best to just leave this module as-is (perhaps adding a top-level allow at most). The hermit folks are the ones who can review safety comments in it well and for now it's not necessary.

I can accept this with the changes reverted and an allow added instead of denys.

I would guess that having the current diff available though would be helpful, so posting a link to a gist or so with the diff seems good.

Thanks!

@maekawatoshiki
Copy link
Contributor Author

This is a link to a gist with diff: https://gist.github.com/maekawatoshiki/ccd255d2e59288917ebee1fb42ba776c

Mark-Simulacrum
Mark-Simulacrum reviewed Aug 29, 2020
View reviewed changes
library/std/src/sys/hermit/thread_local_dtor.rs Outdated
@@ -1,5 +1,6 @@
#![cfg(target_thread_local)]
#![unstable(feature = "thread_local_internals", issue = "none")]
#![allow(unsafe_op_in_unsafe_fn)]
Copy link
Member

Choose a reason for hiding this comment

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

Could we put these in hermit/mod.rs instead?

@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 29, 2020
@crlf0710
Copy link
Member

@maekawatoshiki Ping from triage! Any updates on this?

@maekawatoshiki
Copy link
Contributor Author

I'm a little confused about what to do next...

@crlf0710
Copy link
Member

crlf0710 commented Oct 8, 2020

Triage: @maekawatoshiki I think leaving one single #![allow(unsafe_op_in_unsafe_fn)] at the hermit root is enough. No need to add the others.

@maekawatoshiki
Copy link
Contributor Author

I removed #![allow(unsafe_op_in_unsafe_fn)]s expect for mod.rs.

@Dylan-DPC-zz Dylan-DPC-zz 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 28, 2020
@Dylan-DPC-zz
Copy link

@Mark-Simulacrum this is ready for review

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 5, 2020

📌 Commit 14158f5 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 Nov 5, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 6, 2020
@Dylan-DPC
Rollup merge of rust-lang#74979 - maekawatoshiki:fix, r=Mark-Simulacrum
4f4ce1c 
`#![deny(unsafe_op_in_unsafe_fn)]` in sys/hermit

Partial fix of rust-lang#73904.

This encloses ``unsafe`` operations in ``unsafe fn`` in ``sys/hermit``.
Some unsafe blocks are not well documented because some system-based functions lack documents.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Nov 6, 2020
Closed
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 6, 2020
@JohnTitor
Rollup merge of rust-lang#74979 - maekawatoshiki:fix, r=Mark-Simulacrum
69fe16a 
`#![deny(unsafe_op_in_unsafe_fn)]` in sys/hermit

Partial fix of rust-lang#73904.

This encloses ``unsafe`` operations in ``unsafe fn`` in ``sys/hermit``.
Some unsafe blocks are not well documented because some system-based functions lack documents.
@JohnTitor JohnTitor mentioned this pull request Nov 6, 2020
Closed
@JohnTitor JohnTitor mentioned this pull request Nov 6, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 6, 2020
@bors
Auto merge of rust-lang#78810 - JohnTitor:rollup-8fhtvxu, r=JohnTitor
7e9a36f 
Rollup of 15 pull requests

Successful merges:

 - rust-lang#74979 (`#![deny(unsafe_op_in_unsafe_fn)]` in sys/hermit)
 - rust-lang#78006 (Use Intra-doc links for std::io::buffered)
 - rust-lang#78167 (Fix unreachable sub-branch detection in or-patterns)
 - rust-lang#78514 (Allow using 1/2/3/4 for `x.py setup` options)
 - rust-lang#78538 (BTreeMap: document a curious assumption in test cases)
 - rust-lang#78559 (Add LLVM upgrades from 7 to 10 to RELEASES.md)
 - rust-lang#78666 (Fix shellcheck error)
 - rust-lang#78705 (Print a summary of which test suite failed)
 - rust-lang#78726 (Add link to rust website)
 - rust-lang#78730 (Expand explanation of reverse_bits)
 - rust-lang#78760 (`deny(invalid_codeblock_attributes)` for rustc_error_codes)
 - rust-lang#78771 (inliner: Copy unevaluated constants only after successful inlining)
 - rust-lang#78794 (rustc_expand: use collect_bang helper instead of manual reimplementation)
 - rust-lang#78795 (The renumber pass is long gone)
 - rust-lang#78798 (Fixing Spelling Typos)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4136ed2 into rust-lang:master Nov 6, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.
8 participants
@maekawatoshiki @rust-highfive @Dylan-DPC-zz @Mark-Simulacrum @crlf0710 @bors @joshtriplett @rustbot