seq: parse "infinity" and "-infinity"

[shinhs0506]

No description provided.

[tertsdiepraam]

Hmm, I wonder if this is actually better? I think the comment in the code might have expected the parsing to simplify more than it actually did. The original is pretty clear and does not do redundant parsing of the float. Maybe we can refactor the original to something like this:

let float_val = match s.to_ascii_lowercase() {
   "inf" => ExtendedBigDecimal::Infinity,
   "-inf" => ExtendedBigDecimal::MinusInfinity,
   "nan" | "-nan" => return Err(ParseNumberError::Nan),
   _ => return Err(ParseNumberError::Float)
};

Ok(PreciseNumber::new(float_val, 0, 0))

[shinhs0506]

im just assuming floating point comparison to be cheaper than string comparison. Its just unfortunate that the code doesn't look as clean because match statement is unusable in this case.

[tertsdiepraam]

im just assuming floating point comparison to be cheaper than string comparison.

I'm not so sure actually, because the floating point parsing needs to do string comparisons as well to parse it in the first place. Even though it does it in a clever way: https://doc.rust-lang.org/src/core/num/dec2flt/parse.rs.html#200-243. In any case, if it's just for performance reasons, we need to back it up with benchmarks.

[shinhs0506]

I did some benchmarking and looks like theres no noticeable speed improvement. However, the current code still fails when given "infinty" and "-infinity" as arguments which can be fixed with this pr.

[github-actions]

GNU testsuite comparison:

Skipping an intermittent issue tests/rm/rm1

[github-actions]

GNU testsuite comparison:

Skipping an intermittent issue tests/rm/rm1

[tertsdiepraam]

Good catch on the (-)infinity! That is indeed an improvement and something we should support, however we could do that with the new version I proposed quite easily too.

Why do you think this is better than string comparison?

[shinhs0506]

I don't necessarily think one is better than the other. Scratch what i said about float vs string comparison.
However, with what you have proposed, if IEEE 754 gets updated for some reason, we would also have to update our code accordingly.

[jfinkels]

I wrote that comment originally, but now that I'm looking at it, I also agree with @tertsdiepraam that it's not really any more practical to use f32 parsing than it is to just check for "inf", "nan", etc. as strings. Maybe if we were actually going to use the f32 value here, then it would be useful, but we just return an error in that case anyway. So I suggest we just remove the comment (and possibly take the simplification suggested here: #5124 (comment) )

[shinhs0506]

@jfinkels thanks for the input, i have updated the code

[tertsdiepraam]

Great! Thanks! I'm rerunning the FreeBSD because something is up with it, but it looks ready!