clarify wrapping ptr arithmetic docs #80383
Conversation
|
r? @dtolnay (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
library/core/src/ptr/const_ptr.rs
Outdated
| /// pointer is dereferenced when it is out-of-bounds of the object it is attached to. [`offset`] | ||
| /// can be optimized better and is thus preferable in performance-sensitive code. | ||
| /// | ||
| /// `x.wrapping_offset(o).wrapping_offset(-o)` is always the same as `x` (if `-o` does not |
There was a problem hiding this comment.
Perhaps using x.wrapping_offset(o).wrapping_offset(o.wrapping_neg()) and avoiding the remark about overflow is more clear.
There was a problem hiding this comment.
Oh right... somehow I thought isize::MIN + isize::MIN would not be zero, but indeed it is zero.
library/core/src/ptr/const_ptr.rs
Outdated
| /// is *not* the same as `y`, and dereferencing it is undefined behavior | ||
| /// unless `x` and `y` point into the same allocated object. | ||
| /// In other words, `let z = x.wrapping_add((y as usize).wrapping_sub(x as usize) / | ||
| /// size_of::<T>())` does *not* make `z` the same as `y`: `z` is still attached to the object `x` is |
There was a problem hiding this comment.
I’m not 100% happy with the fact that the first "example code" in the safety remark is one that doesn’t even mention wrapping_offset. The same applies to the documentation of wrapping_sub.
There was a problem hiding this comment.
I guess we could forcefully cast... but that just obscures the point even more as the use of usize::wrapping_sub already does. I'd prefer to just write x.wrapping_add(y - x), since then it is much more clear what is happening.
I did some more editing, what do you think?
There was a problem hiding this comment.
Yes, clarity is the most important thing here (while staying "correct" is only important to avoid confusion). Stating some preconditions on the size of T and overflow, like you did now, is a nice approach.
|
@bors r+ rollup |
|
📌 Commit 8543388 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 11 pull requests Successful merges: - rust-lang#80383 (clarify wrapping ptr arithmetic docs) - rust-lang#80390 (BTreeMap: rename the area access methods) - rust-lang#80393 (Add links to the source for the rustc and rustdoc books.) - rust-lang#80398 (Use raw version of align_of in rc data_offset) - rust-lang#80402 (Document `InferTy` & co.) - rust-lang#80403 (fix: small typo error in chalk/mod.rs) - rust-lang#80410 (rustdoc book: fix example) - rust-lang#80419 (Add regression test for rust-lang#80375) - rust-lang#80430 (Add "length" as doc alias to len methods) - rust-lang#80431 (Add "chr" as doc alias to char::from_u32) - rust-lang#80448 (Fix stabilization version of deque_range feature.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #80306
@steffahn please let me know if this helps avoid the misunderstanding. :)