@oli @eddyb any idea what the purpose of this
parse call is?
the result is thrown away as is the error information, but this does short-circuit in case of error. but then it parses the same thing again anyway?
It parses it again with a different algorithm though. I thought it was intended to cross-validate the two implementations, but i can't explain why it combines them in this way.
there used to be a comment there that referenced an issue, not sure why it got lost or what issue that was
oh you're probably thinking of #31407
code seems to be added by https://github.com/rust-lang/rust/commit/cf103e56bd0bfdc3ef7202ba774a1981adb36a46 (by @oli :D ) and I dont see a comment
@RalfJ that's because it was copied from very old code :P
huh this is missing a FIXME
@oli do you remember why you removed the check that
yeah with such a check I wouldn't have asked
but maybe they actually disagree in practice and this made more programs go through -- as in, https://github.com/rust-lang/rust/issues/31407
if they disagree then we have an issue? but I'd defer to @rkruppe
We'd have a novel (frightening) issue if they both parsed the same string as a different float. We already know they can disagree in that libcore fails to parse something apfloat can parse.
oh so they disagree in which floats they accept, but if they both accept they should give the same result?
Yes. The disagreement about which floats are accepted is a bug (in libcore), but a relatively benign one.
also why do we care if the one in libcore accepts this float at all?^^ is that just serving as "extended testsuite"?
@rkruppe we re-introduced the check that both succeed, are you saying we should also be checking that they both produce the same result in case of success?
If you want better cross-validation, yeah. Accepting the right decimal strings is relatively easy compared to computing the right binary float result.
we should then run a
check crater run w/ the check reintroduced
I guess? I don't think it's terribly likely that this finds any issues (I have relatively high confidence in both implementations now that they're been around long enough and have been cross-validated for a while), but if we're going to compare them at all then comparing the results as well seems more sensible.
also if that line there is really just to test the libcore parser, there should be a comment saying so :)
Not so much testing one implementation as sanity-checking both, but yeah, the intent should be documented so we don't need another thread like this :)