✨Use parsed float value for useless comparison warnings #5030
Conversation
giacomocavalieri
left a comment
giacomocavalieri
left a comment
There was a problem hiding this comment.
This is great, well done!! I left just a tiny nit inline
| float_value: NotNan( | ||
| 1.0, | ||
| ), |
There was a problem hiding this comment.
Isn't this the ONE constant you defined earlier?
| float_value: NotNan( | |
| 1.0, | |
| ), | |
| float_value: NotNan::ONE, |
There was a problem hiding this comment.
This is the snapshot of the parser test, not rust code! 🕵️
There was a problem hiding this comment.
Oh my, sorry for that, I totally missed the context 😆
| 1 │ pub fn main() { 1_0.0 == 10.0 } | ||
| │ ^^^^^^^^^^^^^ This is always `True` |
compiler-core/src/parse.rs
Outdated
| /// A thin f64 wrapper that does not permit NaN. | ||
| /// This allows us to implement `Eq`, which require reflexivity. | ||
| /// | ||
| /// Used for gleam float literals. May contain infinity. |
There was a problem hiding this comment.
If this is used for Gleam float literals it shouldn't contain infinity as well, should it?
There was a problem hiding this comment.
This is true for the Erlang target (and there is a compile time check for it), but on JS you can enter float literals that are too large (e.g. 1.0e500) and they turn into (negative) infinity.
There was a problem hiding this comment.
Aha, now I see, thanks for explaining! Would you mind adding this explanation to the comment? That would be really helpful for future people wondering the same thing
There was a problem hiding this comment.
Good point, done!
fruno-bulax
force-pushed
the
parse-floats
branch
2 times, most recently
from
6fa1918 to
573bd5d
Compare
fruno-bulax
force-pushed
the
parse-floats
branch
from
573bd5d to
d95743a
Compare
lpil
left a comment
lpil
left a comment
There was a problem hiding this comment.
Fab work! This is really nice!
I've left some small notes inline 🙏
compiler-core/src/parse.rs
Outdated
| /// While there is no syntax for "infinity", float literals | ||
| /// may overflow into (possibly negative) infinity on the JS target. | ||
| #[derive(Clone, Copy, Debug, PartialEq)] | ||
| pub struct NotNan(f64); |
There was a problem hiding this comment.
This is a rather confusing name to see in the codebase. Can we call it something like FloatValue or LiteralFloatValue instead 🙏
There was a problem hiding this comment.
Naming, my eternal nemesis. I went for LiteralFloatValue, since that more narrowly describes what it is.
CHANGELOG.md
Outdated
| - Patterns aliasing a string prefix have been optimised to generate faster code | ||
| on the Erlang target. | ||
| ([Giacomo Cavalieri](https://github.com/giacomocavalieri)) | ||
| - Useless comparison warnings for floats now consider the parsed value. |
There was a problem hiding this comment.
Space between changelog entries please
This entry describes the compiler internals, but it doesn't tell the Gleam programmer what has changed from their point of view, which is what this document is for. Could we have that here please 🙏
There was a problem hiding this comment.
Reworded and moved to bug fixes where it actually belongs!
| #[test] | ||
| fn float_literals_redundant_comparison_signed_zero() { | ||
| assert_warning!("pub fn main() { 0.0 == -0.0 }"); | ||
| } |
There was a problem hiding this comment.
Can we have test for floats like 10. please
fruno-bulax
force-pushed
the
parse-floats
branch
2 times, most recently
from
a69fd1c to
e1e23f0
Compare
fruno-bulax
force-pushed
the
parse-floats
branch
from
e1e23f0 to
982b563
Compare
Exhaustiveness checks now correctly account for underscores in ints/floats. Float checks also now used parsed float values defined in PR gleam-lang#5030.
Exhaustiveness checks now correctly account for underscores in ints/floats. Float checks also now used parsed float values defined in PR gleam-lang#5030.
Exhaustiveness checks now correctly account for underscores in ints/floats. Float checks also now used parsed float values defined in PR gleam-lang#5030.
3 participants
Closes #5001
Parallel to the implementation for ints, we're now parsing the literal value and passing it along the AST.
This allows us to do smarter useless comparison warnings instead of relying on string comparison.
To maintain the
Eqtrait impl on many types, I created aNotNantype to wrap f64 that beNaNand can therefore properly implementEqandOrd. I wasn't quite sure where to put the type, I'll gladly move it.I noticed that we still do string-based checks in clauses for both ints and floats, I'll create a follow-up issue after this is merged.