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.

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

docs: size_of is implemented as stride, but claims otherwise #33266

Closed
pnkfelix opened this issue Apr 28, 2016 · 1 comment · Fixed by #33335
Closed

docs: size_of is implemented as stride, but claims otherwise #33266

pnkfelix opened this issue Apr 28, 2016 · 1 comment · Fixed by #33335
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

Discussion of rust-lang/rfcs#1397 with the lang team led to inspection of the documentation for std::mem::size_of and std::intrinsics::size_of

  • std::mem::size_of (link) just says "returns the size of a type in bytes." Understanding what this means (and deciding whether it is true or not) depends on how one interprets the word "size" in that sentence.
  • std::intrinsics::size_of (link) is worse. Why? Because it adds the additional paragraph:

This is the exact number of bytes in memory taken up by a value of the given type. In other words, a memset of this size would exactly overwrite a value. When laid out in vectors and structures there may be additional padding between elements.

This is much more specific, but the added specifics are wrong in terms of that the implementation does.

The quoted text is describing the notion of "size" as denoted by Swift. But Rust's implementation of std::intrinsics::size_of ends up eventually calling out to LLVM's getTypeAllocSize, which:

Returns the offset in bytes between successive objects of the specified type, including alignment padding.


At this point we cannot change the name nor the behavior of std::mem::size_of -- which is fine, since as I understand it, the behavior of std::mem::size_of basically matches that of sizeof(T) in the C/C++ language family.

But we should:

  1. Change the documentation for std::mem::size_of, to make it clear what its behavior is (for this, we might as well point out that it is behaving like "stride" in Swift's terms)
  2. We should either fix the documentation for std::intrinsics::size_of so it no longer outright lies, or replace that single intrinsic with a pair of intrinsics that are analogous to Swift's size and stride.
    • (With respect to Separate size and stride for types rfcs#1397, if we do introduce an analogous distinction, we will have to choose a name other than "size" for the quantity that is not forcibly rounded up to a multiple of the alignment. Maybe "length", "fill", or "utilization"...)

Neither of the above items are incredibly high priority, since the std::mem::size_of docs are not currently lying (just vague), and std::intrinsic::size_of is unstable. But it would probably be good to address at least one of the two, for these reasons:

  • to avoid users making invalid inferences about std::mem::size_of's behavior (and thus manually rounding up its results to each type's alignment), and
  • to avoid some well-meaning Rust developer cut-and-pasting the std::intrinsic::size_of docs over into std::mem::size_of.
@pnkfelix pnkfelix changed the title docs: size_of is implemented as stride but claims otherwise docs: size_of is implemented as stride, but claims otherwise Apr 28, 2016
@pnkfelix pnkfelix added A-docs T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 28, 2016
@brson brson added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Apr 30, 2016
@cramertj
Copy link
Member

cramertj commented May 2, 2016

I'd like to take this one if @pnkfelix is alright with it.

Manishearth added a commit to Manishearth/rust that referenced this issue May 2, 2016
docs: Changed docs for `size_of` to describe size as a stride offset

Current documentation for `std::mem::size_of` is ambiguous, and the documentation for `std::intrinsics::size_of` incorrectly defines size.

This fix re-defines size as the offset in bytes between successive instances of a type, as described in LLVM's [getTypeAllocSize](http://llvm.org/docs/doxygen/html/classllvm_1_1DataLayout.html#a1d6fcc02e91ba24510aba42660c90e29).

Fixes: rust-lang#33266
Manishearth added a commit to Manishearth/rust that referenced this issue May 3, 2016
docs: Changed docs for `size_of` to describe size as a stride offset

Current documentation for `std::mem::size_of` is ambiguous, and the documentation for `std::intrinsics::size_of` incorrectly defines size.

This fix re-defines size as the offset in bytes between successive instances of a type, as described in LLVM's [getTypeAllocSize](http://llvm.org/docs/doxygen/html/classllvm_1_1DataLayout.html#a1d6fcc02e91ba24510aba42660c90e29).

Fixes: rust-lang#33266
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants