Document From::from impls
#137330
Document From::from impls
#137330
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
O-linux
|
The Miri subtree was changed cc @rust-lang/miri Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead. cc @calebzulawski, @programmerjake rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
|
@rustbot label A-docs C-enhancement T-libs-api |
rustbot
added
A-docs
|
Should I move Portable SIMD and rust-analyzer changes even though they are small? |
This comment has been minimized.
This comment has been minimized.
|
It is generally a good idea to split up large changes. :) You changed a whole bunch of files in rust-analyzer, so yeah that should be split. Miri and clippy have fewer changes, but the fact that there are any changes at all there still contradicts the PR description. It's too late now, but the best strategy for PRs like this is to start with just a few files, e.g. just |
|
The Miri subtree was changed cc @rust-lang/miri Some changes occurred in src/tools/clippy cc @rust-lang/clippy Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead. cc @calebzulawski, @programmerjake rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer |
|
My mistake, I will revert the rust-analyzer changes. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rustbot
added
has-merge-commits
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Oli Scherer <[email protected]>
src/tools/clippy/clippy_lints/src/attrs/mixed_attributes_style.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Alejandra González <[email protected]>
|
r? libs There are to many impls I haven't made much review progress |
There was a problem hiding this comment.
I do not believe you have self-reviewed this adequately, and I do not agree with the direction of this PR. Please first remove all instances where the documentation is trivial... for example, for From<X> for Y, the documentation is most certainly trivial when it is "Wraps X in Y"... or for only internal-facing APIs.
| /// Converts a SIMD mask to an array of bools. | ||
| /// | ||
| /// ## Cost | ||
| /// Copies the bytes of the array |
There was a problem hiding this comment.
incorrect and furthermore misleading, as the real cost is the usage of instructions for converting the mask types to the boolean values, which can be extremely expensive relative to merely moving the bytes around. these may perform essentially arbitrary computation to extract the mask from the opaque representation into a sequence of bytes where each byte is exactly 0x00 or 0x01.
There was a problem hiding this comment.
it's still relatively cheap, but merely moving bytes can easily be, well, the easy part, and for people who care about the cost of something done in SIMD, it can be one of the highest costs in their program!
| @@ -109,6 +109,7 @@ pub trait Wake { | |||
| impl<W: Wake + Send + Sync + 'static> From<Arc<W>> for Waker { | |||
| /// Use a [`Wake`]-able type as a `Waker`. | |||
| /// | |||
| /// ## Cost | |||
There was a problem hiding this comment.
I do not agree with this new "Cost" heading
There was a problem hiding this comment.
Why so, I think it makes it more clear.
rustbot
added
S-waiting-on-author
There was a problem hiding this comment.
It's too late now,
I do not think it is too late. Factoring this PR to make it reviewable is now a blocking consideration for this PR now that it is in my queue. I'm sorry you had to wait to hear that, @TimTheBig, but this PR has to be reviewed fairly carefully to make sure that it does not present anything that could be misinterpreted as new stable guarantees about how these APIs work, unless they are trivially obvious to guarantee. In many cases these are actually not as obvious to me, so I am going to insist that this is well-organized, at minimum.
The entire library API surface change of this should be separated from anything that documents only internal-facing APIs or only affects things in src/tools
|
@programmerjake, @oli-obk, @blyxyas, @workingjubilee, thanks for all the help. |
Co-authored-by: Jubilee <[email protected]>
This resolves #51430 by documenting all
fromimpls in lib std and core.I still need someone to look over a few of the comments to see if the style is correct.