Update README#252
Conversation
…k to the blog post describing safety
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
Generally I'm happy with this, but there are a few points from the review feedback I would like to re-validate before we land this.
Thanks!
| //! as possible using SIMD instructions. | ||
| //! These can be created in a SIMD context using the [`SimdFrom`] trait, or the | ||
| //! [`from_slice`][SimdBase::from_slice] associated function. | ||
| //! ## Automatic vectorization |
There was a problem hiding this comment.
Nit:
| //! ## Automatic vectorization | |
| //! # Automatic vectorization |
Edit: Oh, you seem to have done this throughout the file. I'm interested to hear why this was needed!
There was a problem hiding this comment.
First, it's the obviously correct heading structure given the level 1 header in the README.
Second, it looks a little nicer when rendered.
But it's not a hill I'm willing to die on so if you have strong opinions on header levels please go ahead and change them.
There was a problem hiding this comment.
Could you point specifically to this header? In my GitHub diff viewer, it seems to go from h1 straight to h3...
Cargo rdme adds a level to the headings, so as to allow both to be idiomatic.
| //! implementation, use the [`dispatch`] macro: | ||
| //! Put the code to vectorize in an `#[inline(always)]` function generic over [`Simd`]. | ||
| //! | ||
| //! This will generate several implementations for different SIMD levels and select the best one at runtime: |
There was a problem hiding this comment.
Personally, I'd mention dispatch here. Something like:
| //! This will generate several implementations for different SIMD levels and select the best one at runtime: | |
| //! You can then call this function using the `dispatch` macro. | |
| //! This will generate several implementations for different SIMD levels and select the best one at runtime: |
There was a problem hiding this comment.
I think it's already covered in prose in the inlining section in detail. I don't think duplicating only a part of it here would be beneficial.
There was a problem hiding this comment.
My point was more that the prose here to me reads that just writing the generic function is what makes the multiple implementations, which just isn't the case. But I don't care too much; this is still better than before.
| //! Fearless SIMD uses "marker values" which serve as proofs of which target features are available on the current CPU. | ||
| //! These each implement the [`Simd`] trait, which exposes a core set of SIMD operations which are implemented as | ||
| //! efficiently as possible on each target platform. | ||
| //! Zero dependencies, from-scratch build time under 1 second, and >99.8% safe Rust under the hood! |
There was a problem hiding this comment.
I don't think that the ">99.8% safe Rust" clause reads especially well. I think it's trying to express two things:
- That we're sound.
- That you can use the same abstractions to access the same functionality safely.
For the former, that's the default expectation for a Rust library, and so I wouldn't choose to use it as an "entry-point marketing" thing. I think a prose section about how we maintain soundness is valuable, but luckily that's already covered by your article.
For the latter, this is a valuable differentiator. I would phrase it as something like "choose your own level of abstraction" instead, but that is already kind of covered by the sentence beforehand anyway.
(As a note, this whole section isn't something I'd normally expect to find in Linebender's tone of voice, but I'm happy for this to be in your style).
There was a problem hiding this comment.
What I am trying to convey is that very little unsafe is used internally, so the library is easy to audit, and doesn't have a whole lot that can go wrong.
This is a crucial differentiator because all other SIMD abstractions except jxl-simd have thousands of unsafe blocks.
There was a problem hiding this comment.
I've changed it, please take another look.
…stration/pay wall. Reword the tagline and link to it. Link to a specific part of the article from the inlining section.
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
Barring the two blocking concerns (header levels and ungrammatical sentence), this looks good to me. Thanks!
I especially really like the idea of having examples for each level.
| //! implementation, use the [`dispatch`] macro: | ||
| //! Put the code to vectorize in an `#[inline(always)]` function generic over [`Simd`]. | ||
| //! | ||
| //! This will generate several implementations for different SIMD levels and select the best one at runtime: |
There was a problem hiding this comment.
My point was more that the prose here to me reads that just writing the generic function is what makes the multiple implementations, which just isn't the case. But I don't care too much; this is still better than before.
2 participants
Remove "experimental" warning from the README, brag about our offerings, and add an example for every abstraction level.