Stream: t-compiler/const-eval

Topic: parse_float


RalfJ (May 26 2019 at 09:47, on Zulip):

@oli @eddyb any idea what the purpose of this parse call is?
https://github.com/rust-lang/rust/blob/2268d9923bcb34a6c921d285cca7fa3dba857c02/src/librustc_mir/hair/constant.rs#L89
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?

rkruppe (May 26 2019 at 09:49, on Zulip):

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.

oli (May 26 2019 at 10:51, on Zulip):

there used to be a comment there that referenced an issue, not sure why it got lost or what issue that was

rkruppe (May 26 2019 at 11:24, on Zulip):

oh you're probably thinking of #31407

RalfJ (May 26 2019 at 11:47, on Zulip):

code seems to be added by https://github.com/rust-lang/rust/commit/cf103e56bd0bfdc3ef7202ba774a1981adb36a46 (by @oli :D ) and I dont see a comment

eddyb (May 28 2019 at 14:36, on Zulip):

@RalfJ that's because it was copied from very old code :P

eddyb (May 28 2019 at 14:39, on Zulip):

https://github.com/rust-lang/rust/blob/e999e7b8b2e35a495d6b9630ab987c0703c6ab48/src/librustc_const_math/float.rs#L79-L89

eddyb (May 28 2019 at 14:39, on Zulip):

huh this is missing a FIXME

eddyb (May 28 2019 at 14:42, on Zulip):

@oli do you remember why you removed the check that libcore and rustc_apfloat agree?

RalfJ (May 28 2019 at 14:55, on Zulip):

yeah with such a check I wouldn't have asked

RalfJ (May 28 2019 at 14:55, on Zulip):

but maybe they actually disagree in practice and this made more programs go through -- as in, https://github.com/rust-lang/rust/issues/31407

eddyb (May 28 2019 at 14:56, on Zulip):

if they disagree then we have an issue? but I'd defer to @rkruppe

rkruppe (May 28 2019 at 15:06, on Zulip):

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.

RalfJ (May 28 2019 at 15:07, on Zulip):

oh so they disagree in which floats they accept, but if they both accept they should give the same result?

rkruppe (May 28 2019 at 15:07, on Zulip):

Yes. The disagreement about which floats are accepted is a bug (in libcore), but a relatively benign one.

RalfJ (May 28 2019 at 15:07, on Zulip):

also why do we care if the one in libcore accepts this float at all?^^ is that just serving as "extended testsuite"?

eddyb (May 28 2019 at 15:08, on Zulip):

@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?

rkruppe (May 28 2019 at 15:09, on Zulip):

If you want better cross-validation, yeah. Accepting the right decimal strings is relatively easy compared to computing the right binary float result.

eddyb (May 28 2019 at 15:09, on Zulip):

we should then run a check crater run w/ the check reintroduced

rkruppe (May 28 2019 at 15:11, on Zulip):

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.

RalfJ (May 28 2019 at 15:18, on Zulip):

also if that line there is really just to test the libcore parser, there should be a comment saying so :)

rkruppe (May 28 2019 at 15:24, on Zulip):

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 :)

Last update: Nov 15 2019 at 20:05UTC