RFR (S) JDK-8043301: Duplicate definitions in vm/runtime/sharedRuntimeTrans.cpp versus math.h in VS2013

[Vladimir Kozlov]

On 5/21/14 11:03 AM, Lois Foltan wrote:
>
> On 5/21/2014 1:47 PM, Vladimir Kozlov wrote:
>> These methods are there to get the same results on all platforms.
>>
>> I don't like adding 'A'. Can we use __scalbn names?
>>
>> Also we need to ask SAP if they have conflict with new names before
>> removing #if.
>
> Thanks Vladimir for the review.  I hesitate to go to the use of double
> underscores since I thought this was reserved for a C/C++ compiler's
> internal use.   Also one underscore is how copysign is defined within
> VS2010's math.h so that is not a viable option either.  Again, there was
> precedent within sharedRuntimeTrig.cpp for the 'A', so I chose to follow
> suit.

Yes, it is a pain to find unique name. Using namespace could solve this 
too but we are not using it in VM :(

So we have duplicated code in these files. In this case can you move 
copysignA, scalbnA and other common definitions into .hpp file?

>
> I would be happy to follow up with SAP, do you have a specific contact,
> or by cc'ing the ppc-aix-port-dev alias that should be sufficient?

CC to ppc-aix-port-dev should get their attention. If they will not 
comment, we will assume it is okay to change.

Thanks,
Vladimir

>
> Lois
>
>> Thanks,
>> Vladimir
>>
>> On 5/21/14 10:22 AM, Lois Foltan wrote:
>>>
>>> On 5/21/2014 12:49 PM, Tim Bell wrote:
>>>> Lois:
>>>>
>>>>> Please review the following fix:
>>>>>
>>>>> Webrev:
>>>>>     http://cr.openjdk.java.net/~lfoltan/bug_jdk8043301/
>>>>>
>>>>> Bug: Duplicate definitions in vm/runtime/sharedRuntimeTrans.cpp
>>>>> versus math.h in VS2013
>>>>>     https://bugs.openjdk.java.net/browse/JDK-8043301
>>>>>
>>>>> Summary of fix:
>>>>> VS2013's include/math.h now contains definitions for both copysign()
>>>>> and scalbn().  This yielded compilation conflicts with the JVM's
>>>>> definition of these functions within runtime/sharedRuntimeTrans.cpp.
>>>>> I changed copysign and scalbn definitions to be consistent with how
>>>>> they are defined within sharedRuntimeTrig.cpp.  They are now defined
>>>>> as copysignA and scalbnA static functions.  I also removed the
>>>>> !defined(AIX) conditionalization around these defintions since no
>>>>> conflict should now exist with AIX's math.h.
>>>>>
>>>>> Tests:
>>>>>     JPRT build & test, Hotspot jtreg, JDK lang & util,
>>>>> vm.quick.testlist (in progress)
>>>>
>>>> Thank you for the fix.  Looks good to me, although I am not a hotspot
>>>> reviewer or committer.  I have very low confidence that their
>>>> implementations of 'copysign' and 'scalbn' would exactly match what we
>>>> use on all other platforms, especially since we can't inspect their
>>>> code.
>>>
>>> Thanks Tim for the review.  I think due to performance reasons
>>> sharedRuntimeTrans.cpp and sharedRuntimeTrig.cpp have their own
>>> definitions.   These are the only two files in the JVM that use copysign
>>> and scalbn.
>>>
>>>>
>>>> I have the sharedRuntimeTrans.cpp patch in my compiler upgrade source
>>>> tree... testing now.
>>>
>>> Great, thanks again!
>>> Lois
>>>
>>>>
>>>> Tim
>>>>
>>>
>