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

Rollup of 6 pull requests #74084

Closed
wants to merge 17 commits into from

Conversation

Manishearth
Copy link
Member

Successful merges:

Failed merges:

r? @ghost

JohnTitor and others added 17 commits July 2, 2020 15:18
This commit marks temporaries from MIR construction as internal such
that they are skipped in `sanitize_witness` (where each MIR local is
checked to have been contained within the generator interior computed
during typeck). This resolves an ICE whereby the construction of checked
addition introduced a `(u64, bool)` temporary which was not in the HIR
and thus not in the generator interior.

Signed-off-by: David Wood <david@davidtw.co>
to feature(rc_as_ptr)

These were stabilized alongside the Weak versions,
but having `feature = "weak_.."` on a fn definition
for the non-weak pointers is potentially very confusing.
Previously, if there were a module in scope with the same name as the
primitive, that would take precedence. Coupled with
rust-lang#58699, this made it impossible
to link to the primitive when that module was in scope.

This approach could be extended so that `struct@foo` would no longer resolve
to any type, etc. However, it could not be used for glob imports:

```rust
pub mod foo {
  pub struct Bar;
}

pub enum Bar {}
use foo::*;

// This is expected to link to `inner::Bar`, but instead it will link to the enum.
/// Link to [struct@Bar]
pub struct MyDocs;
```

The reason for this is that this change does not affect the resolution
algorithm of rustc_resolve at all. The only reason we could special-case
primitives is because we have a list of all possible primitives ahead of time.
Add Integer::checked_{log,log2,log10}

This implements `{log,log2,log10}` methods for all integer types. The implementation was provided by @substack for use in the stdlib.

_Note: I'm not big on math, so this PR is a best effort written with limited knowledge. It's likely I'll be getting things wrong, but happy to learn and correct. Please bare with me._

## Motivation
Calculating the logarithm of a number is a generally useful operation. Currently the stdlib only provides implementations for floats, which means that if we want to calculate the logarithm for an integer we have to cast it to a float and then back to an int.

> would be nice if there was an integer log2 instead of having to either use the f32 version or leading_zeros() which i have to verify the results of every time to be sure

_— [@substack, 2020-03-08](https://twitter.com/substack/status/1236445105197727744)_

At higher numbers converting from an integer to a float we also risk overflows. This means that Rust currently only provides log operations for a limited set of integers.

The process of doing log operations by converting between floats and integers is also prone to rounding errors. In the following example we're trying to calculate `base10` for an integer. We might try and calculate the `base2` for the values, and attempt [a base swap](https://www.rapidtables.com/math/algebra/Logarithm.html#log-rules) to arrive at `base10`. However because we're performing intermediate rounding we arrive at the wrong result:

```rust
// log10(900) = ~2.95 = 2
dbg!(900f32.log10() as u64);

// log base change rule: logb(x) = logc(x) / logc(b)
// log2(900) / log2(10) = 9/3 = 3
dbg!((900f32.log2() as u64) / (10f32.log2() as u64));
```
_[playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6bd6c68b3539e400f9ca4fdc6fc2eed0)_

This is somewhat nuanced as a lot of the time it'll work well, but in real world code this could lead to some hard to track bugs. By providing correct log implementations directly on integers we can help prevent errors around this.

## Implementation notes

I checked whether LLVM intrinsics existed before implementing this, and none exist yet. ~~Also I couldn't really find a better way to write the `ilog` function. One option would be to make it a private method on the number, but I didn't see any precedent for that. I also didn't know where to best place the tests, so I added them to the bottom of the file. Even though they might seem like quite a lot they take no time to execute.~~

## References

- [Log rules](https://www.rapidtables.com/math/algebra/Logarithm.html#log-rules)
- [Rounding error playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6bd6c68b3539e400f9ca4fdc6fc2eed0)
- [substack's tweet asking about integer log2 in the stdlib](https://twitter.com/substack/status/1236445105197727744)
- [Integer Logarithm, A. Jaffer 2008](https://people.csail.mit.edu/jaffer/III/ilog.pdf)
…bank

Audit hidden/short code suggestions

Should fix rust-lang#73641.
Audit uses of `span_suggestion_short` and `tool_only_span_suggestion` (`span_suggestion_hidden` is already tested with `run-rustfix`). Leave some FIXMEs for futher improvements/fixes.
r? @estebank
…mp-generator-interior, r=matthewjasper

mir: mark mir construction temporaries as internal

Fixes rust-lang#73914.

This PR marks temporaries from MIR construction as internal such that they are skipped in `sanitize_witness` (where each MIR local is checked to have been contained within the generator interior computed during typeck). This resolves an ICE whereby the construction of checked addition introduced a `(u64, bool)` temporary which was not in the HIR and thus not in the generator interior.

r? @matthewjasper
Move A|Rc::as_ptr from feature(weak_into_raw) to feature(rc_as_ptr)

These were stabilized alongside the Weak versions, but having `feature = "weak_.."` on a fn definition for the non-weak pointers is potentially very misleading, especially in a review context where the impl header may not be immediately visible.

r? @RalfJung
@bors rollup=always
…oli-obk

Miri value validation: fix handling of uninit memory

Fixes rust-lang/miri#1456
Fixes rust-lang/miri#1467

r? @oli-obk
Always resolve type@primitive as a primitive, not a module

Previously, if there were a module in scope with the same name as the
primitive, that would take precedence. Coupled with
rust-lang#58699, this made it impossible
to link to the primitive when that module was in scope.

This approach could be extended so that `struct@foo` would no longer resolve
to any type, etc. However, it could not be used for glob imports:

```rust
pub mod foo {
  pub struct Bar;
}

pub enum Bar {}
use foo::*;

// This is expected to link to `inner::Bar`, but instead it will link to the enum.
/// Link to [struct@Bar]
pub struct MyDocs;
```

The reason for this is that this change does not affect the resolution
algorithm of rustc_resolve at all. The only reason we could special-case
primitives is because we have a list of all possible primitives ahead of time.

Closes rust-lang#74063

r? @Manishearth
@Manishearth
Copy link
Member Author

@bors r+ p=5 rollup=never

@bors
Copy link
Contributor

bors commented Jul 6, 2020

📌 Commit 346a147 has been approved by Manishearth

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 6, 2020
@JohnTitor JohnTitor added the rollup A PR which is a rollup label Jul 6, 2020
@Manishearth
Copy link
Member Author

@bors retry r+

@bors
Copy link
Contributor

bors commented Jul 6, 2020

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Jul 6, 2020

📌 Commit 346a147 has been approved by Manishearth

@bors
Copy link
Contributor

bors commented Jul 6, 2020

⌛ Testing commit 346a147 with merge 7ba0056d5b438dfddd84b3f4e733d0f9082eac2a...

@bors
Copy link
Contributor

bors commented Jul 6, 2020

💔 Test failed - checks-actions

@bors 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 Jul 6, 2020
@CAD97
Copy link
Contributor

CAD97 commented Jul 6, 2020

failures: 
---- num::int_log::checked_log2 stdout ---- 
thread 'num::int_log::checked_log2' panicked at 'assertion failed: `(left == right)` 
left: `Some(13)`, 
right: `Some(12)`', src/libcore/../libcore/tests/num/int_log.rs:37:9 


failures: 
num::int_log::checked_log2

#70835

@Manishearth Manishearth closed this Jul 6, 2020
@Manishearth Manishearth deleted the rollup-szyclqo branch July 18, 2020 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rollup A PR which is a rollup S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants