Rephrase docs in for ptr

[CreepySkeleton]

These methods can be supplied with NULL just fine, this is the whole point of Option<&T> return type.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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.

[Centril]

r? @RalfJung

[jonas-schievink]

I think that's what the sentence was saying before, no?

if the pointer is non-NULL

[CreepySkeleton]

Oh, yeah, right. I would still advise you to change the wording, it's really easy to misread is as it is

[CreepySkeleton]

What do you think?

[rust-highfive]

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-11-13T17:57:41.9335710Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-13T17:57:41.9529078Z ##[command]git config gc.auto 0
2019-11-13T17:57:41.9590200Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-13T17:57:42.5615464Z ##[command]git config --get-all http.proxy
2019-11-13T17:57:42.5623290Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66379/merge:refs/remotes/pull/66379/merge
---
2019-11-13T18:04:12.4229439Z Found 0 error codes with no tests
2019-11-13T18:04:12.4229488Z Done!
2019-11-13T18:04:12.4229546Z 
2019-11-13T18:04:12.4229593Z 
2019-11-13T18:04:12.4231667Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-11-13T18:04:12.4231833Z 
2019-11-13T18:04:12.4231861Z 
2019-11-13T18:04:12.4237773Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-11-13T18:04:12.4237868Z Build completed unsuccessfully in 0:01:39
2019-11-13T18:04:12.4237868Z Build completed unsuccessfully in 0:01:39
2019-11-13T18:04:12.4294484Z == clock drift check ==
2019-11-13T18:04:12.4315222Z   local time: Wed Nov 13 18:04:12 UTC 2019
2019-11-13T18:04:12.5821548Z   network time: Wed, 13 Nov 2019 18:04:12 GMT
2019-11-13T18:04:12.5826974Z == end clock drift check ==
2019-11-13T18:04:13.8882637Z 
2019-11-13T18:04:13.9008829Z ##[error]Bash exited with code '1'.
2019-11-13T18:04:13.9039347Z ##[section]Starting: Checkout
2019-11-13T18:04:13.9042240Z ==============================================================================
2019-11-13T18:04:13.9042319Z Task         : Get sources
2019-11-13T18:04:13.9042366Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[CreepySkeleton]

Also, could you guys please clarify what " dereferencable for the whole size of T" means? I thought that any pointer is dereferencable, i.e we can dereference it inside an unsafe block.

[the8472]

In an unsafe block being able to write the code is not the same as it being valid. See
https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html

Raw pointers must point to valid data when you dereference them, it's up to the programmer to ensure that they are dereferencable at that point in time.

[RalfJung]

Also, could you guys please clarify what " dereferencable for the whole size of T" means?

Dereferencable isn't a syntactic property, it says whether the pointer actually points to some allocated memory. Like, ptr::null() is not dereferencable. If you try, you have UB.

I thought that any pointer is dereferencable, i.e we can dereference it inside an unsafe block.

That's like saying "every car can fly, i.e., we can throw it out of an airplane". ;)

[RalfJung]

More seriously though, we define "dereferencable" at https://doc.rust-lang.org/nightly/core/ptr/index.html. @CreepySkeleton does that help? Maybe the docs here should link there?

[RalfJung]

Given that these return references, we should probably also mention something about aliasing?

For as_ptr that would be something like "the memory the pointer points to is not mutated until the lifetime is over (except inside UnsafeCell)", and for the mutable variant something like "the memory the pointer points to is not accessed (read or written) by any other pointer until the lifetime is over".

[CreepySkeleton]

it says whether the pointer actually points to some allocated memory.

But "it points to an initialized instance of T" implies that already. If it's a separate entry in the list than it should supply a reader with some new information, while it doesn't which is confusing. I would suggest to elaborate here and replace this two entries with a single

• it is dereferencable, i.e the memory range of the given size_of::<T>() starting at the pointer must all be within the bounds of a single allocated object (possibly stack allocated) and this object must be an initialized instance of T

[RalfJung]

But "it points to an initialized instance of T" implies that already.

Fair.

it is dereferencable, i.e the memory range of the given size_of::() starting at the pointer must all be within the bounds of a single allocated object (possibly stack allocated) and this object must be an initialized instance of T

That's not correct. "dereferencable" is a technical term and just means the memory is allocated; on its own it does not say anything about the data there being initialized or so.

[RalfJung]

I think the reason "dereferencable" is listed separately is that the validity part is explicitly called out as "speculative", while dereferencable is not speculative at all. Maybe we should just remove that remark, and just say something like

it must point to an initialized instance of T; in particular, the pointer must be "dereferencable" for size size_of::<T>() in the sense defined here.

[CreepySkeleton]

@RalfJung Could you please explain term "speculative"?

[RalfJung]

Could you please explain term "speculative"?

This is UB because we want to be conservative, and if we allow it now we can never make it UB in the future. At rust-lang/unsafe-code-guidelines#77 we are discussing if, long-term, we really want this to be UB.
It is also "spec-only" UB right now, in the sense that the compiler doesn't actually do anything that would rely on this being UB. Code that causes "spec-only" UB works, but still violates the spec and might stop working in the future.

[RalfJung]

@CreepySkeleton could you rebase and try using intra-doc links? I guess the target would be crate::ptr#safety.

[GuillaumeGomez]

Should be good now. :3

[RalfJung]

Indeed CI is finally happy. :)

@bors r+ rollup+

[bors]

📌 Commit f11dd32 has been approved by RalfJung