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

use `MIN`/`MAX` constant names in integer pattern coverage messages #57073

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@zackmdavis
Copy link
Member

zackmdavis commented Dec 23, 2018

While many programmers may intuitively appreciate the significance of magic numbers like −2147483648, Rust is about empowering everyone to build reliable and efficient software! It's a bit more legible to print the constant names.

The max_as_i128, &c. methods strewn in libsyntax are a bit unsightly, but I fear this is really the most natural way to solve the problem.

Question: is it OK to have chosen ::std::isize::MIN (&c.) as the symbolic names, or will no_std crate authors be mad??

Resolves #56393.

r? @varkor

use `MIN`/`MAX` constant names in integer pattern coverage messages
While many programmers may intuitively appreciate the significance of
magic numbers like −2147483648, Rust is about empowering everyone to
build reliable and efficient software! It's a bit more legible to
print the constant names.

The `max_as_i128`, &c. methods strewn in libsyntax are a bit
unsightly, but I fear this is really the most natural way to solve the
problem.

Resolves #56393.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 23, 2018

The job x86_64-gnu-llvm-6.0 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.
travis_time:end:00a64580:start=1545542584805937420,finish=1545542585833384030,duration=1027446610
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
travis_time:start:test_mir-opt
Check compiletest suite=mir-opt mode=mir-opt (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:05:26] 
[01:05:26] running 38 tests
[01:05:44]     let mut _12: bool;
[01:05:44]     let mut _13: bool;
[01:05:44]     let mut _14: bool;
[01:05:44]     let mut _15: bool;
[01:05:44]     let mut _16: bool;
[01:05:44]     let mut _17: u32;
[01:05:44]     let mut _18: u32;
[01:05:44]     bb0: {                              
[01:05:44]         StorageLive(_2);
[01:05:44]         StorageLive(_3);
[01:05:44]         StorageLive(_4);
[01:05:44]         StorageLive(_5);
[01:05:44]         StorageLive(_6);
[01:05:44]         StorageLive(_7);
[01:05:44]         StorageLive(_8);
[01:05:44]         _8 = _1;
[01:05:44]         _7 = const compiler_builtins::int::addsub::rust_i128_add(move _8, const 1i128) -> bb7;
[01:05:44]     }
[01:05:44]     bb1: {                              
[01:05:44]         _10 = Eq(const 4i128, const -1i128);
[01:05:44]         _11 = Eq(_5, const ::std::i128::MIN);
[01:05:44]         _12 = BitAnd(move _10, move _11);
[01:05:44]         assert(!move _12, "attempt to divide with overflow") -> bb2;
[01:05:44]     }
[01:05:44]     bb2: {                              
[01:05:44]         _4 = const compiler_builtins::int::sdiv::rust_i128_div(move _5, const 4i128) -> bb8;
[01:05:44]     }
[01:05:44]     bb3: {                              
[01:05:44]         _14 = Eq(const 5i128, const -1i128);
[01:05:44]         _15 = Eq(_4, const ::std::i128::MIN);
[01:05:44]         _16 = BitAnd(move _14, move _15);
[01:05:44]         assert(!move _16, "attempt to calculate the remainder with overflow") -> bb4;
[01:05:44]     }
[01:05:44]     bb4: {                              
[01:05:44]         _3 = const compiler_builtins::int::sderflow") -> bb7;
[01:05:44]     }
[01:05:44]     bb7: {                              
[01:05:44]         _3 = const compiler_builtins::int::sdiv::rust_i128_rem(move _4, const 5i128) -> bb15;
[01:05:44]     }
[01:05:44]     bb8: {                              
[01:05:44]         _2 = move (_20.0: i128);
[01:05:44]         StorageDead(_3);
[01:05:44]         StorageLive(_23);
[01:05:44]         _23 = const 7i32 as u128 (Misc);
[01:05:44]         _21 = const compiler_builtins::int::shift::rust_i128_shro(move _2, move _23) -> bb16;
[01:05:44]     }
[01:05:44]     bb9: {                              
[01:05:44]         _0 = move (_21.0: i128);
[01:05:44]         StorageDead(_2);
[01:05:44]         return;
[01:05:44]     }
[01:05:44]     bb10: {                             
[01:05:44]         assert(!move (_9.1: bool), "attempt to add with overflow") -> bb1;
[01:05:44]     }
[01:05:44]     bb11: {                             
[01:05:44]         assert(!move (_10.1: bool), "attempt to subtract with overflow") -> bb2;
[01:05:44]     }
[01:05:44]     bb12: {                             
[01:05:44]         assert(!move (_11.1: bool), "attempt to multiply with overflow") -> bb3;
[01:05:44]     }
[01:05:44]     bb13: {                             
[01:05:44]         StorageDead(_5);
[01:05:44]         _16 = Eq(const 5i128, const 0i128);
[01:05:44]         assert(!move _16, "attempt to calculate the remainder with a divisor of zero") -> bb6;
[01:05:44]     }
[01:05:44]     bb14: {                             
[01:05:44]         StorageDead(_22);
[01:05:44]         assert(!move (_20.1: bool), "attempt to sht-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:05:44] 
[01:05:44] 
[01:05:44] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:505:22
[01:05:44] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:05:44] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:05:44] Build completed unsuccessfully in 0:10:24
[01:05:44] Makefile:58: recipe for target 'check' failed
[01:05:44] make: *** [check] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:19da1ec5
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun Dec 23 06:28:58 UTC 2018
---
travis_time:end:2332a858:start=1545546539890449956,finish=1545546539950413353,duration=59963397
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:082269c9
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:169f99e9
$ dmesg | grep -i kill

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)

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 23, 2018

Question: is it OK to have chosen ::std::isize::MIN (&c.) as the symbolic names, or will no_std crate authors be mad?

Using ::core::isize::MIN should work flawlessly on Rust 2018 otherwise if you want to use core.
My personal preference is to use core but I don't mind if you use std.

@@ -10,11 +10,11 @@ note: lint level defined here
LL | #![deny(unreachable_patterns)]
| ^^^^^^^^^^^^^^^^^^^^

error[E0004]: non-exhaustive patterns: `128u8..=255u8` not covered
error[E0004]: non-exhaustive patterns: `128u8..=::std::u8::MAX` not covered

This comment has been minimized.

@nikic

nikic Dec 23, 2018

Contributor

There's quite a lot of syntax in here (..=::) making this a bit hard to read. Could we get away with not including the ::std:: prefix and just using 128u..=u8::MAX? I realize this may not work if the user decides to directly copy&paste this to create an extra range, but it would reduce the noise in the message.

This comment has been minimized.

@varkor

varkor Dec 23, 2018

Member

I agree that it's quite noisy, but considering that the compiler doesn't currently suggest importing std::u8 if u8::MAX is not found, it could be confusing that it's not possible to directly copy and paste. Maybe we could start with this more verbose version and then cut it down after implementing a suggestion for importing std::u8.

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Dec 23, 2018

Question: is it OK to have chosen ::std::isize::MIN (&c.) as the symbolic names, or will no_std crate authors be mad??

I think we should detect whether no_std is used and give the correct crate root prefix (e.g. by using resolve_str_path or something similar).

@varkor
Copy link
Member

varkor left a comment

Nice! Just a few minor comments.

@@ -1458,6 +1458,29 @@ impl IntTy {
IntTy::I128 => 128,
})
}

pub fn min_as_i128(&self) -> i128 {

This comment has been minimized.

@varkor

varkor Dec 23, 2018

Member

You can avoid enumerating the integer types using the bit manipulations that are already used for range pattern matching, e.g.:

// FIXME(49937): refactor these bit manipulations into interpret.
let bits = Integer::from_attr(&cx.tcx, SignedInt(ity)).size().bits() as u128;
let min = 1u128 << (bits - 1);
let max = (1u128 << (bits - 1)) - 1;

This comment has been minimized.

@varkor

varkor Dec 23, 2018

Member

Although these should be refactored as part of #49937 anyway, it probably makes more sense to have these in mir/mod.rs if that's the only place they're used.


pub fn min_as_i128(&self) -> i128 {
match *self {
IntTy::Isize => ::std::isize::MIN as i128,

This comment has been minimized.

@varkor

varkor Dec 23, 2018

Member

This is going to use the host isize size, rather than the target size. (Using the snippet above will address that.)

@@ -1496,6 +1519,17 @@ impl UintTy {
UintTy::U128 => 128,
})
}

pub fn max_as_u128(&self) -> u128 {

This comment has been minimized.

@varkor

varkor Dec 23, 2018

Member

Relevant snippet here:

// FIXME(49937): refactor these bit manipulations into interpret.
let bits = Integer::from_attr(&cx.tcx, UnsignedInt(uty)).size().bits() as u128;
let max = !0u128 >> (128 - bits);

Uint(ui) => return write!(f, "{:?}{}", bits, ui),
Uint(ui) => {
return match bits {
// writing 0 as uX::MIN wouldn't clarify

This comment has been minimized.

@varkor

varkor Dec 23, 2018

Member
Suggested change Beta
// writing 0 as uX::MIN wouldn't clarify
// Writing `0` as `uX::MIN` wouldn't make the value any clearer,
// so we only special-case the maximum value.
return write!(f, "{:?}{}", ((bits as i128) << shift) >> shift, i);
let n = ((bits as i128) << shift) >> shift;
return match n {
m if m == i.min_as_i128() => write!(f, "::std::{}::MIN", i),

This comment has been minimized.

@varkor

varkor Dec 23, 2018

Member
Suggested change Beta
m if m == i.min_as_i128() => write!(f, "::std::{}::MIN", i),
_ if n == i.min_as_i128() => write!(f, "::std::{}::MIN", i),

This is just stylistic, but I think it's clearer what value you're testing if you don't mind n to a different variable name (here and above).

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 26, 2018

☔️ The latest upstream changes (presumably #57108) made this pull request unmergeable. Please resolve the merge conflicts.

@Aaronepower

This comment has been minimized.

Copy link
Contributor

Aaronepower commented Jan 11, 2019

Triage; @zackmdavis Hello, have you been able to get back to this PR?

@zackmdavis

This comment has been minimized.

Copy link
Member

zackmdavis commented Jan 11, 2019

@Aaronepower estimate Sunday the 13th

@zackmdavis

This comment has been minimized.

Copy link
Member

zackmdavis commented Jan 14, 2019

estimate Sunday the 13th

Actually not, but this is in my awareness/TODO list

@TimNN TimNN added A-allocators and removed A-allocators labels Jan 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment