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

seq: compute correct width for -0e0 #2657

Closed

Conversation

jfinkels
Copy link
Collaborator

This adds code similar to the code in print_seq_integers() that correctly supports negative zero as a starting number when the negative zero is given in the input in scientific notation.

Before this change:

$ ./target/debug/coreutils seq -w -0e0 1
00
01

After this change:

$ ./target/debug/coreutils seq -w -0e0 1
-0
01

That matches GNU seq:

$ seq -w -0e0 1
-0
01

@jfinkels
Copy link
Collaborator Author

jfinkels commented Sep 11, 2021

I don't know why the CI is complaining about this new test. It works on my machine (Ubuntu 18.04, rustc 1.55.0):

$ cargo run seq -w -0e0 1
    Finished dev [unoptimized + debuginfo] target(s) in 0.22s
     Running `target/debug/coreutils seq -w -0e0 1`
-0
01
$ cargo test test_seq::test_width_negative_zero_scientific_notation
[...]
running 1 test
test test_seq::test_width_negative_zero_scientific_notation ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 1400 filtered out; finished in 0.00s

@jfinkels jfinkels force-pushed the seq-negative-zero-width-float branch 5 times, most recently from d25bc41 to d8c751a Compare September 17, 2021 02:15
Compute the correct width when an input argument is `-0e0`, that is,
floating point negative zero in scientific notation. For example,

    $ seq -w -0e0 1
    -0
    01
@jfinkels
Copy link
Collaborator Author

I see. The CI build is using an older version of rustc. I found that rustc v1.52.0 causes this new test to fail:

$ rustup toolchain install 1.52.0 && cargo +1.52.0 test test_seq::test_width_negative_zero_scientific_notation
[...]
---- test_seq::test_width_negative_zero_scientific_notation stdout ----
current_directory_resolved: 
run: /home/jeffrey/src/coreutils/target/debug/coreutils seq -w -0e0 1
thread 'test_seq::test_width_negative_zero_scientific_notation' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :
<"-0\n1\n"
>"-0\n01\n"

', tests/common/util.rs:229:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

And v1.53.0 causes the test to pass:

$ rustup toolchain install 1.53.0 && cargo +1.53.0 test test_seq::test_width_negative_zero_scientific_notation
[...]
running 1 test
test test_seq::test_width_negative_zero_scientific_notation ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 1412 filtered out; finished in 0.00s

I'll try to figure out what change I can make to get this code to work.

@jfinkels
Copy link
Collaborator Author

Okay, I've figured it out. Consider this Rust program:

fn main() {
    println!("{}", (-0.0f64).to_string());
}

When compiled with rustc v1.52.1 and rustc v1.53.0, we get different outputs:

$ cargo +1.52.1 run
   Compiling negativezero v0.1.0 (/tmp/negativezero)
    Finished dev [unoptimized + debuginfo] target(s) in 0.92s
     Running `target/debug/negativezero`
0
$ cargo +1.53.0 run
   Compiling negativezero v0.1.0 (/tmp/negativezero)
    Finished dev [unoptimized + debuginfo] target(s) in 0.89s
     Running `target/debug/negativezero`
-0

I believe this is due to this change rust-lang/rust#78618, which appears in v1.53.0: https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1530-2021-06-17

Since my other pull request, #2613, replaces this part of the code with something that doesn't suffer from this problem, I'm just going to close this pull request and make sure the unit test is included in that other branch.

@jfinkels jfinkels closed this Sep 18, 2021
@jfinkels jfinkels deleted the seq-negative-zero-width-float branch September 18, 2021 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant