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

Add range lint for float literals #14519

Merged
merged 1 commit into from
Jul 10, 2014
Merged

Conversation

hirschenberger
Copy link
Contributor

Issue #10934

@kud1ing
Copy link

kud1ing commented May 30, 2014

Very nice. I think it would be extra helpful to also print the limit which is exceed.

@hirschenberger
Copy link
Contributor Author

It looks pretty ugly for f64:

warning: literal out of range for its type  ( > 179769313486231680088648464220646842686668242844028646442228680066046004606080400844208228060084840044686866242482868202680268820402884062800406622428864666882406066422426822086680426404402040202424880224808280820888844286620802664406086660842040886824002682662666864246642840408646468824200860804260804068888)

Is there some formatstring to limit ints and floats to a maximal width, automaically switching to e-Notation (something like 'g' in printf)?

@ftxqxd
Copy link
Contributor

ftxqxd commented May 31, 2014

@hirschenberger Perhaps you’re looking for {:e}?

println!("{:1.3e}", std::f64::MAX_VALUE);
// 1.798e308

This does format 1.0 as 1.000e0, but that shouldn’t be a problem.

@hirschenberger
Copy link
Contributor Author

I know the e modifier but it doesn't work with i64 which also is too long to display. Would it make sense to implement the UpperExp and LowerExp trait for integers?

@@ -12,6 +12,9 @@

#![doc(primitive = "f64")]

// FIXME: MIN_VALUE and MAX_VALUE literals are parsed as -inf and inf #14353
#![allow(type_overflow)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you restrict this to just the statics in question?

@hirschenberger
Copy link
Contributor Author

I have a new version that also prints the boundary that was exceeded using scientific notation for floats and decimal notation for integers. Do you think it's sensible @alexcrichton ?

@alexcrichton
Copy link
Member

Yes the way it's printed now is ok.

@hirschenberger
Copy link
Contributor Author

The Problem is, that the tests are failing. As you can see in the Travis output the lint function displays a positive boundary when the f64 is negative.
I don't know why this happens only on f64. The test is lit_val > max or lit_val < min which should also work for nan and -nan values.
I also tested a version with lit_val.is_infinite() && lit_val.is_positive() or lit_val.is_infinite() && lit_val.is_negative() but I got the same result. In a standalone testprogram everything works as expected:

use std::f32;
use std::f64;

fn main()
{
  let o = from_str::<f64>("-1.7976931348623158e+308").unwrap();
  if o.is_negative() && o.is_infinite() {
    println!("OK {}", o);
  }
  else {
    println!("NOK {}", o);
  }
}

I have no clue.

@alexcrichton
Copy link
Member

I'm not sure why that test would be failing, but it also looks like you'll need to add back in #[deny(warnings)] because the test is also failing by compiling successfully.

You could drop the < foo or > foo messages as well possibly.

@hirschenberger
Copy link
Contributor Author

Yes I know the failure by successful compilation, I changed it here locally but the test still fails because of the f64 issue I mentioned above.
I really would like to know why the comparison fails in the lint code. I've build a debug version of rustc and try my luck with gdb.

@hirschenberger
Copy link
Contributor Author

I stepped through the debugger and the problem is that from_str (strconv.rs) returns a nan instead of a -nan. The from_str_bytes_common function seems broken in various border cases (see also #14353).
Can't we just use libc::funcs::c95::stdlib::strtod and friends for the parsing stuff and add rust's additional syntax sugar around it?

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with a rebase!

@hirschenberger
Copy link
Contributor Author

@alexcrichton The floatparsing stuff is blocking this, I'm willing to work on this but I don't know which solution would be accepted. Would a rewrite of the flawed from_str_bytes_common function using the libc-functions make sense or should I try to fix the existing code?

@alexcrichton
Copy link
Member

I believe fixing the existing code would be the best option.

@hirschenberger
Copy link
Contributor Author

In #14353 I suggested some ways to fix the code, can you tell me what's your favourite solution?

@alexcrichton
Copy link
Member

I haven't been able to look too closely at our floating-point parsing code, but I imagine that an uninvasive solution would work pretty well regardless.

@emberian
Copy link
Member

emberian commented Jul 7, 2014

Reopening by request.

@emberian emberian reopened this Jul 7, 2014
@hirschenberger
Copy link
Contributor Author

I changed the implementation back to not display the exceeded range due to MIN_VALUE and MAX_VALUE parsing issues.

r? @alexcrichton

@hirschenberger
Copy link
Contributor Author

It seems the testvalues where a rounded differently on macs, increased the exceeding values.

@hirschenberger
Copy link
Contributor Author

The last build error seems unrelated, can you retry after I rebased?

@bors bors merged commit f8bc571 into rust-lang:master Jul 10, 2014
@hirschenberger hirschenberger deleted the issue-10934 branch July 10, 2014 19:01
@hirschenberger
Copy link
Contributor Author

I think it can be closed @alexcrichton

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2023
internal: Don't recreate Hygiene unnecessarily
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.

6 participants