Page MenuHomePhabricator

Eina: implement strtod in C locale and remove linkl against msvcr100.
ClosedPublic

Authored by vtorri on Feb 13 2019, 4:01 AM.

Details

Summary

This fixes compilation on Windows

More precisely edje_cc could not compile emotion edc files, so it was a runtime problem
because of msvcr100 link.

Add more tests than before

Test Plan

compilation

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
vtorri created this revision.Feb 13 2019, 4:01 AM
vtorri requested review of this revision.Feb 13 2019, 4:01 AM

I've check the compilation on Windows (MSYS2, mingw-w64, autotools) and Fedora. Compilation was fine on both OS. On Fedora, tests fail (when compared to strtod) on some rare values (around 2e-308) and for NAN

Please fix the sibject line to fit into a reasonable line width (72-80). The rest can easily go into the normal commit message body.

Is there anything we can do about the failing tests compared to strtod? The implementation here is not accurate enough for some cases? Does that come as price of portability or is this a bug to be fixed?
Thanks for adding tests btw!

about the bugs, it's certainly something which can be fixed. but instead of having 0, this implementation return 2E-308, or the contrary (not a big deal imho, especially for a function which is never used).

I have no idea how to modify the subject line with arc.

I would think its the git commits itself which has the long subject line. Fixing it there would also fix an updated patch to phab with arc.

As for the tests, I had failures with eina 5 out of 5 tries with this patch applied. Either we only test that part when the bug is fixed or we have a fixed implementation.
Letting code and tests in that will fail knowingly is not an option.

on fedora, in the eina suite, only eina_convert fails.
I can comment the tests which fail if you want :-)

about the first test which fails, it's not a problem of value, it's a problem of errno value which is not the same (no errno set contrary to strtod which sets errno to ERANGE)

https://stackoverflow.com/questions/2499329/strtod-and-sprintf-inconsistency-under-gcc-and-msvc

it seems that whatever fix I do, i will never be able to implement a strtod so that it gives the same results than all the possible imlementations of strtod in the world (glibc, Windows libc, clang, newlib, freebsd, musl, maybe cygwin too, etc...).

I think that some tests are anyway not useful (the ones which try to correctly parse doubles with large absolute exponent)

what do you think ?

keep the tests simple then - don't go too far in trying to test it as it just can't pass on all platforms. like you say - with large numbers it might not be that useful etc.

vtorri updated this revision to Diff 19476.Wed, Feb 20, 9:10 PM

Updating D7926: Eina: implement strtod in C locale and remove linkl against msvcr100.

This fixes compilation on Windows More precisely edje_cc could not
compile emotion edc files, so it was a runtime problem because of
msvcr100 link.
Add more tests than before

zmike retitled this revision from Eina: implement strtod in C locale and remove linkl against msvcr100. This fixes compilation on Windows More precisely edje_cc could not compile emotion edc files, so it was a runtime problem because of msvcr100 link. Add more tests than before to Eina: implement strtod in C locale and remove linkl against msvcr100..Mon, Feb 25, 4:37 AM
zmike edited the summary of this revision. (Show Details)
vtorri edited the summary of this revision. (Show Details)Mon, Feb 25, 4:42 AM

see inline questions :)

src/lib/eina/eina_convert.c
463

any reason to not just use tolower() ?

src/tests/eina/eina_test_convert.c
208–254

A question - is changing the tests to compare against the system strtod() with C locale the right thing to do here? might there possibly be rounding errors along the way? e.g. x86 supports 80bits in FP registers. shouldn't this be all done very carefully and checked within a small range of error and not exactly? just wondering if some compiler flags or future optimizations might not break these tests the way they are written?

vtorri added inline comments.Wed, Feb 27, 4:50 AM
src/lib/eina/eina_convert.c
463

i test only what i want. I can use tolower if you want, not a problem for me.

src/tests/eina/eina_test_convert.c
208–254

i don't know if it is the right thing to do . I can use EINA_DBL_EQ (eina_util.h) maybe

zmike added a subscriber: zmike.Wed, Feb 27, 12:29 PM

This seems pretty reasonable overall.

For additional testing, I replaced all existing strtod usage in the efl tree with this one. Things worked as expected.

Minor changes needed, but overall I'm satisfied.

src/lib/eina/eina_convert.c
463

I'd prefer to see tolower (or at least eina_str_tolower) here unless there's a functional reason to NIH the function again.

src/tests/eina/eina_test_convert.c
208–254

If you're going to use == then maybe using eina_dbl_exact is best?

I think going with EINA_DBL_EQ is probably sensible here to avoid failures with unusual archs/compilers/cflags or hipster libc implementations in the future.

vtorri added inline comments.Wed, Feb 27, 1:20 PM
src/lib/eina/eina_convert.c
463

ok, i'll use tolower()

src/tests/eina/eina_test_convert.c
208–254

thinking a bit more, == or eina_dbl_exact() have the same problem. I'll use EINA_DBL_EQ

vtorri updated this revision to Diff 19791.Wed, Feb 27, 11:45 PM
  • use tolower() instead of my own implementation
vtorri marked an inline comment as done.Wed, Feb 27, 11:46 PM
vtorri updated this revision to Diff 19802.Thu, Feb 28, 1:46 AM
  • eina_test_convert: use EINA_DBL_EQ instead of comparing double's with ==
vtorri marked an inline comment as done.Thu, Feb 28, 1:47 AM
vtorri updated this revision to Diff 19803.Thu, Feb 28, 1:59 AM
  • use EINA_DBL_EQ to compare double (bis)
stefan_schmidt accepted this revision.Thu, Feb 28, 5:27 AM
This revision is now accepted and ready to land.Thu, Feb 28, 5:27 AM
This revision was automatically updated to reflect the committed changes.