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

[Vladimir Kozlov]

Yes, this looks good.

Thanks,
Vladimir

On 5/21/14 1:12 PM, Lois Foltan wrote:
>
> On 5/21/2014 2:32 PM, Vladimir Kozlov wrote:
>> 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?
>
> Hi Vladimir,
>
> Good idea, please review updated webrev at
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8043301_2/.  I have verified
> that this builds with VS2010 and VS2013. Retesting is in progress.
>
> Thanks,
> Lois
>
>>
>>>
>>> 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
>>>>>>
>>>>>
>>>
>