ICU-23357 measunit_extra.cpp: Fix error: 'abs' ambiguous

[Dave-Allured]

measunit_extra.cpp, lines 687-689:
This code is used to check an input string for a non-integer value.
With some older compilers, this generates "error: call to 'abs' is ambiguous".

        uint64_t int_result = static_cast<uint64_t>(double_result);
        const double kTolerance = 1e-9;
        if (abs(double_result - int_result) > kTolerance) {

This PR will avoid the "abs" error by replacing the handcrafted fractional test, with appropriate use of "modf".

I am not experienced with C++. Someone please check this work, thank you.

Downstream issue with complete build log: https://trac.macports.org/ticket/73705
Platform was: macOS 10.14.6 (darwin/18.7.0) arch i386
Compiler was: Apple LLVM version 10.0.1 (clang-1001.0.46.4)
Dialect flag was: -std=gnu++11

TODO: Please read the following on ICU Contributing, and then delete these instructions.
-- Did not read. If this PR is not tolerable, let me know and I will fix.

• For general info on contributing: https://github.com/unicode-org/icu/blob/main/CONTRIBUTING.md
  -- Did not read.

Checklist

• Required: Issue filed: ICU-23357
• Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Issue accepted (done by Technical Committee after discussion)
• Tests included, if applicable
• [N/A] API docs and/or User Guide docs changed or added, if applicable
• Approver: Feel free to merge on my behalf

[CLAassistant]

All committers have signed the CLA.

[markusicu]

@Dave-Allured could you please create a Jira ticket for this issue? https://icu.unicode.org/bugs

@eggrobin or @roubert could you please take a look at the math & standard library calls here? (very small change...)

[Dave-Allured]

@Dave-Allured could you please create a Jira ticket for this issue?

Done.
https://unicode-org.atlassian.net/browse/ICU-23357

[Dave-Allured]

My change is now rewritten to use the modf() function correctly. Here is a revised overall summary of this PR.

• The previous method used a cast to integer, followed by subtraction, to split double_result into integral and fractional parts.

• This new code uses the standard modf() function to perform the same split in a single function call.

In my opinion, the standard function is a slightly more robust method than the previous "handcrafted" fractional test. However, I readily admit that the previous method performed well, and did not seem to have any numerical flaws.

The irony is that my entire motivation for this change is simply compatibility with a few older compilers. This now seems like a lot of trouble to fix those older compiles.

At least I can say that this avoids the original mixed-type expression (double minus uint64_t), and the mysterious "ambiguous" error, which I never did understand where that was coming from.

[roubert]

OK, now the header file and function call are correct and all tests on the CI pass.

[Dave-Allured]

@roubert Thank you.

Note to admin: I am not able to use the git command line because of an authentication problem. Therefore I am using github only.for PR's. Github does not currently provide a squash function. Therefore I must request that you use squash-merge when merging this. Thanks again for your assistance.

[eggrobin]

The original error message in the issue is unsettling, as it is suggesting that abs(double) is not visible (otherwise it would win over those integer overloads):

:info:build measunit_extra.cpp:689:13: error: call to 'abs' is ambiguous
:info:build         if (abs(double_result - int_result) > kTolerance) {
:info:build             ^~~
:info:build /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/usr/include/stdlib.h:132:6: note: candidate function
:info:build int      abs(int) __pure2;
:info:build          ^
:info:build /Library/Developer/CommandLineTools/usr/include/c++/v1/stdlib.h:111:44: note: candidate function
:info:build inline _LIBCPP_INLINE_VISIBILITY long      abs(     long __x) _NOEXCEPT {return  labs(__x);}
:info:build                                            ^
:info:build /Library/Developer/CommandLineTools/usr/include/c++/v1/stdlib.h:113:44: note: candidate function
:info:build inline _LIBCPP_INLINE_VISIBILITY long long abs(long long __x) _NOEXCEPT {return llabs(__x);}
:info:build                                            ^
:info:build 1 error generated.
:info:build gnumake[1]: *** [measunit_extra.ao] Error 1
:info:build gnumake[1]: *** Waiting for unfinished jobs....
:info:build gnumake[1]: Leaving directory `/opt/local/var/macports/build/icu-52f0161a/work/icu/source/i18n'
:info:build gnumake: *** [all-recursive] Error 2
:info:build gnumake: Leaving directory `/opt/local/var/macports/build/icu-52f0161a/work/icu/source'
:info:build Command failed:  cd "/opt/local/var/macports/build/icu-52f0161a/work/icu/source" && /usr/bin/gnumake -j8 -w all VERBOSE=1 
:info:build Exit code: 2
:error:build Failed to build icu: command execution failed
:debug:build Error code: CHILDSTATUS 2921 2

The fix should be as simple as just including <math.h> so abs(double) is visible, no need to change the code.

[Dave-Allured]

@eggrobin Thank you for your assessment and deeper analysis of my original problem. Missing header, now I see that.

Let us make a final choice. You said "no need to change". However, as I said, I think that the purpose-built modf function is slightly preferable to the original "handcrafted" method to split integral and fractional parts. Do you think this is sufficient to justify my proposed code change?

[eggrobin]

I think that the purpose-built modf function is slightly preferable to the original "handcrafted" method to split integral and fractional parts. Do you think this is sufficient to justify my proposed code change?

Yes.

[Dave-Allured]

Now I suspect there may be some corner cases where legitimate integer input strings are falsely rejected by this test. I am converting this to Draft until I can finish checking.