Fix msvc 12.0 builds #6052
Fix msvc 12.0 builds #6052
Conversation
|
Yes, it works on it's own. Though it's best if it gets merged earlier to avoid merge conflicts. |
py/modmath.c
Outdated
| if (x < MICROPY_FLOAT_CONST(0.0)) { | ||
| return -(same_sign ? three_pi_over_four : pi_over_four); | ||
| } | ||
| return same_sign ? pi_over_four : three_pi_over_four; |
There was a problem hiding this comment.
Is it simpler to write it like: sign(x) * (y < 0 ? three_pi_over_four : pi_over_four)?
There was a problem hiding this comment.
Even better: copysign(y < 0 ? three_pi_over_four : pi_over_four, x)
There was a problem hiding this comment.
Nice, didn't think of copysign.
py/modmath.c
Outdated
| mp_float_t msvc_atan2(mp_float_t x, mp_float_t y) { | ||
| if (isinf(x) && isinf(y)) { | ||
| static const mp_float_t pi_over_four = MICROPY_FLOAT_CONST(M_PI_4); | ||
| static const mp_float_t three_pi_over_four = MICROPY_FLOAT_CONST(3.0) * MICROPY_FLOAT_CONST(M_PI_4); |
There was a problem hiding this comment.
Will this introduce some error in computing 3pi/4 due to the mult? And will it do the mult at runtime? Might be better to actually write the full constant here...?
There was a problem hiding this comment.
The constants are calculated at compile time for msvc, no idea if standard guarantees that though so to make sure a constant would be better. No experience with that in practice, so the question is: what is an authorative source for that constant? Also to answer the question wrt whether the compile-time calculated value has an error; the constant I get now is 2.3561944901923448. Which also happens to have a lot of search engine matches but that doesn't mean much.
There was a problem hiding this comment.
The value to lots of decimal places is 2.356194490192344928846982537459627163147877049531329365731. I tested computing it via 3.0 * M_PI / 4.0 and 3.0 * M_PI_4 (actual computation at runtime) and it gets the right answer. But I wouldn't want to rely on this.
It looks like it's enough to truncate it at 2.3561944901923449 but it might be good to use 2.35619449019234492885 to match the precision of MP_PI defined at the top of modmath.c.
There was a problem hiding this comment.
Ok but I'm interested in knowing how you get a constant like that. Plus I need the M_PI_4 one itself as well.
There was a problem hiding this comment.
You can try https://www.wolframalpha.com/ . Or find PI somewhere on the internet to many places (eg the benchmark in this repo called bm_pidigts.py), convert to an integer (a very large one like 31415926), then apply //4 or whatever factor is needed as an integer operation.
There was a problem hiding this comment.
Ha, forgot about wolframalpha. Great site.
|
It looks ok, except the changes to modmath are a bit intrusive. It's nice to support MSVC out-of-the-box but perhaps there's a neater way to fix these bugs. How do other projects deal with it? Looking at CPython it seems that it has explicit code to handle the infinity/nan cases of most functions, including It's not uncommon that a platform has problems with float functions (eg embedded Linux machines with old/custom toolchains). So perhaps these fixes can be made more generic and can be selected by a config? Eg |
Yes. Probably goes unnoticed there and for other projects because most projects' unittests compare against 0 without checking sign. It's only for x64 builds actually, I'll add that explicitly.
Checked some other projects and seems the special handling of nan/inf is rather the rule, not the exception.
I'd say yes, but in this case that would add 3 separate flags, ok? Personally I'm fine with that: MidroPython sure does have a lot of flags already, but that's simply unavoidable when supporting multiple platforms so I don't really care about adding more. |
Yes I think it's fair enough to have 3 new flags for this. They are not really user-facing flags (like ones that enable a feature), instead they are for someone doing a new port, if their build fails some math tests they can enable these flags to make the tests pass. |
Older implementations deal with infinity/negative zero incorrectly. Add fixes and tests.
Older versions do not have inline so fetch definition from mpconfigport.h.
Add configuration which otherwise has to be set via the UI so the file is more self-contained, and remove configuration which is not needed because it's the same as the default. The major change here is that for a while now Appveyor has been using Visual Studio 2015 by default while we still want to support 2013.
|
Ok, all changes applied. Changed the negative zero handling to use copysign as well as that reduces code size a tiny bit I guess. |
…n-main Translations update from Hosted Weblate
This fixes a bunch of issues which went undiscovered for quite a while because Appveyor switched to msvc 14.0 as the default, and switches Appveyor back to use version 12.0