Make Layout's align a NonZeroUsize #51226

gnzlbg · 2018-05-30T19:03:19Z

This PR makes the Layout's align field a NonZeroUsize since it cannot ever be zero, not even while building a Layout. It also contains some drive-by minor cleanups over the docs and the code, like updating the documented error types, or using the size() and align() methods instead of accessing the fields directly (the latter was required for the NonZeroUsize change anyways).

r? @SimonSapin

cc @Amanieu

SimonSapin

Looks good, thanks! Just one thing:

SimonSapin · 2018-05-30T19:59:59Z

src/libcore/alloc.rs

    #[inline]
    pub fn padding_needed_for(&self, align: usize) -> usize {
+        // **FIXME**: This function is only called with proper power-of-two
+        // alignments. Maybe we should turn this into a real assert!.
+        debug_assert!(align.is_power_of_two());


Given that docs say “The return value of this function has no meaning if align is not a power-of-two.” I think this assertion should not be added. (Though in practice this line is a no-op today since the standard library is always compiled in release mode.)

The FIXME hints at this. Basically, I found those docs really weird. If it is a precondition to call this with a non-power-of-two align, then it should be asserted (or the function should be unsafe and the behavior undefined), or it should return Option.

Are there any other functions in standard that are safe and return meaningless results like this one? Maybe I am missing some context.

It is not undefined behavior to call this method with a non-power-of-two, that only causes it to return an integer value that is not meaningful. So it shouldn’t be unsafe IMO. Calling it a precondition really depends on what you mean by that.

It is not undefined behavior to call this method with a non-power-of-two, that only causes it to return an integer value that is not meaningful.

It is undefined if we say it is. That allows us to return a meaningless value, or assert, or abort, or... Maybe I meant implementation defined here, but for implementation defined we need to define what the behavior actually is. With undefined we can do anything, including returning a meaningless value.

Calling it a precondition really depends on what you mean by that.

A precondition on the result having any meaning. AFAICT most APIs in std use Option::None to indicate that a result is meaningless, but maybe there are some APIs already that just return meaningless usizes already?

In any case, as long std tests are built in debug mode and run by some build bot, the debug_assert should tell us if this behavior is relied upon anywhere in std or not.

It is undefined if we say it is.

Sure, but why would we? This method only does integer arithmetic.

Anyway, I’m ready to r+ this PR as soon as this chunk is removed. Maybe the design or existence of this method should be revisited, but in the middle of an unrelated PR is not the place to do it.

gnzlbg · 2018-06-02T11:28:42Z

@SimonSapin I’ll remove the debug assert and ask about it in the tracking issue.

…

On Sat 2. Jun 2018 at 12:50, Simon Sapin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/libcore/alloc.rs <#51226 (comment)>: > #[inline] pub fn padding_needed_for(&self, align: usize) -> usize { + // **FIXME**: This function is only called with proper power-of-two + // alignments. Maybe we should turn this into a real assert!. + debug_assert!(align.is_power_of_two()); It is undefined if we say it is. Sure, but why would we? This method only does integer arithmetic. Anyway, I’m ready to r+ this PR as soon as this chunk is removed. Maybe the design or existence of this method should be revisited, but in the middle of an unrelated PR is not the place to do it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#51226 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3Npi6MNz5TtTsrKarvDDaAKmxmWTw8ks5t4m38gaJpZM4UT1v4> .

rust-highfive · 2018-06-02T14:26:51Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (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.

[00:04:34]    Compiling rustc_asan v0.0.0 (file:///checkout/src/librustc_asan)
[00:04:35]    Compiling rustc_msan v0.0.0 (file:///checkout/src/librustc_msan)
[00:04:35]    Compiling rustc_lsan v0.0.0 (file:///checkout/src/librustc_lsan)
[00:04:36]    Compiling rustc_tsan v0.0.0 (file:///checkout/src/librustc_tsan)
[00:04:37] error[E0615]: attempted to take value of method `align` on type `&alloc::Layout`
[00:04:37]    --> libcore/alloc.rs:251:68
[00:04:37]     |
[00:04:37] 251 |             Ok((Layout::from_size_align_unchecked(alloc_size, self.align)?, padded_size))
[00:04:37]     |
[00:04:37]     |
[00:04:37]     = help: maybe a `()` to call it is missing?
[00:04:37] 
[00:04:37] error[E0277]: the `?` operator can only be applied to values that implement `ops::try::Try`
[00:04:37]    --> libcore/alloc.rs:251:17
[00:04:37]     |
[00:04:37] 251 |             Ok((Layout::from_size_align_unchecked(alloc_size, self.align)?, padded_size))
[00:04:37]     |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `?` operator cannot be applied to type `alloc::Layout`
[00:04:37]     |
[00:04:37]     = help: the trait `ops::try::Try` is not implemented for `alloc::Layout`
[00:04:37] note: required by `ops::try::Try::into_result`
[00:04:37]    --> libcore/ops/try.rs:50:5
[00:04:37]     |
[00:04:37] 50  |     fn into_result(self) -> Result<Self::Ok, Self::Error>;
[00:04:37] 
[00:04:37] error: aborting due to 2 previous errors
[00:04:37] 
[00:04:37] Some errors occurred: E0277, E0615.

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)

SimonSapin · 2018-06-02T23:30:55Z

@bors r+

bors · 2018-06-02T23:30:56Z

📌 Commit 2d7cd70 has been approved by SimonSapin

@SimonSapin

Make Layout's align a NonZeroUsize This PR makes the `Layout`'s align field a `NonZeroUsize` since it cannot ever be zero, not even while building a `Layout`. It also contains some drive-by minor cleanups over the docs and the code, like updating the documented error types, or using the `size()` and `align()` methods instead of accessing the fields directly (the latter was required for the `NonZeroUsize` change anyways). r? @SimonSapin cc @Amanieu

Rollup of 6 pull requests Successful merges: - #51143 (Specify that packed types must derive, not implement, Copy) - #51226 (Make Layout's align a NonZeroUsize) - #51297 (Fix run button style) - #51306 (impl Default for &mut str) - #51312 (Clarify the difference between get_mut and into_mut for OccupiedEntry) - #51313 (use type name in E0599 enum variant suggestion) Failed merges:

bors · 2018-06-03T06:12:58Z

⌛ Testing commit 2d7cd70 with merge 3575be6...

bors · 2018-06-03T10:12:58Z

💥 Test timed out

make Layout's align a NonZeroUsize

6c3ebcd

rust-highfive assigned SimonSapin May 30, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2018

SimonSapin approved these changes May 30, 2018

View reviewed changes

remove debug_assert in padding_needed_for

6ff67ee

gnzlbg mentioned this pull request Jun 2, 2018

Simplify HashMap layout calculation by using Layout #51163

Merged

add missing inline's and optimizations

2d7cd70

gnzlbg force-pushed the nonzero_align branch from e4a7cd4 to 2d7cd70 Compare June 2, 2018 14:46

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 2, 2018

Mark-Simulacrum mentioned this pull request Jun 3, 2018

Rollup of 6 pull requests #51319

Merged

bors merged commit 2d7cd70 into rust-lang:master Jun 3, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 3, 2018

Amanieu mentioned this pull request Jun 5, 2018

Optimize layout calculations in HashMap #51340

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Layout's align a NonZeroUsize #51226

Make Layout's align a NonZeroUsize #51226

gnzlbg commented May 30, 2018

SimonSapin left a comment

SimonSapin May 30, 2018

gnzlbg Jun 1, 2018

SimonSapin Jun 1, 2018

gnzlbg Jun 2, 2018

SimonSapin Jun 2, 2018

gnzlbg commented Jun 2, 2018 via email

rust-highfive commented Jun 2, 2018

SimonSapin commented Jun 2, 2018

bors commented Jun 2, 2018

bors commented Jun 3, 2018

bors commented Jun 3, 2018

Make Layout's align a NonZeroUsize #51226

Make Layout's align a NonZeroUsize #51226

Conversation

gnzlbg commented May 30, 2018

SimonSapin left a comment

Choose a reason for hiding this comment

SimonSapin May 30, 2018

Choose a reason for hiding this comment

gnzlbg Jun 1, 2018

Choose a reason for hiding this comment

SimonSapin Jun 1, 2018

Choose a reason for hiding this comment

gnzlbg Jun 2, 2018

Choose a reason for hiding this comment

SimonSapin Jun 2, 2018

Choose a reason for hiding this comment

gnzlbg commented Jun 2, 2018 via email

rust-highfive commented Jun 2, 2018

SimonSapin commented Jun 2, 2018

bors commented Jun 2, 2018

bors commented Jun 3, 2018

bors commented Jun 3, 2018