[test] Fix FixedPointConversion tests #23220
Conversation
jrose-apple
requested review from
stephentyrone
and removed request for
gribozavr
|
@swift-ci please test |
drodriguez
force-pushed
the
fixed-point-conversion-tests
branch
from
1ae5d04 to
b4f9922
Compare
Fix several problems with FixedPointConversion generation code. The first problem is that at some point `repr(value)` was being used, which turn the number into a string. That was great for printing the number, but make the test against the value of the number (like `testValue < otherMin` always false. There were a number of tests that were never performed, specifically the integer tests. The second problem was using doubles in the Python code. For Float32 and Float64 the tests were generated correctly, but in the case of Float80, the test adding or removing a quantity to the maximum/minimum were failing because of the lack of precission (Adding 0.1 to a very big/small number is the same as not adding anything). Switching to Decimal should keep enough precission for the tests. Finally the last problem was that the bounds of the conversions are not actually `selfMin` and `selfMax`, but the values returned by the utility function `getFtoIBounds`. For example for unsigned types, the lower bound is always -1, not zero (every value between -1 and zero is rounded to zero, and doesn't fail). Instead of using nested gyb templates, use lit.cfg %target-ptrsize, which should be faster, cleaner, and provides correct line-directive output. Remove a bunch of warnings in Swift when compiling the generated result of FixedPointConversion.swift.gyb. Co-authored-by: Gwynne Raskind <[email protected]>
drodriguez
force-pushed
the
fixed-point-conversion-tests
branch
from
b4f9922 to
14d89ec
Compare
|
@stephentyrone : I think I incorporated all the changes from Gwynne in the other PR. Do you mind restarting the CI and reviewing? Thanks! /cc @gwynne |
|
@swift-ci please test |
|
Build failed |
|
Looks great to me! |
|
@swift-ci please test |
|
Build failed |
|
@stephentyrone: can I/you merge this if there’s no objections? |
|
@drodriguez Yeah, I'll give it a more careful review at some point, but I'm OK with taking it now. |
|
@stephentyrone – This change caused a 2.25x regression in |
|
@davezarzycki I'm fine with that, but I won't have a chance to do it today. If you want to put up a PR, I can review. |
Fix several problems with FixedPointConversion generation code.
The first problem is that at some point
repr(value)was being used,which turn the number into a string. That was great for printing the
number, but make the test against the value of the number (like
testValue < otherMinalways false. There were a number of tests thatwere never performed, specifically the integer tests.
The second problem was using doubles in the Python code. For Float32 and
Float64 the tests were generated correctly, but in the case of Float80,
the test adding or removing a quantity to the maximum/minimum were
failing because of the lack of precission (Adding 0.1 to a very
big/small number is the same as not adding anything). Switching to
Decimal should keep enough precission for the tests.
Finally the last problem was that the bounds of the conversions are not
actually
selfMinandselfMax, but the values returned by the utilityfunction
getFtoIBounds. For example for unsigned types, the lowerbound is always -1, not zero (every value between -1 and zero is rounded
to zero, and doesn't fail).
/cc @ultramiraculous