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

[Lois Foltan]

Thanks Coleen for the follow up review!
Lois

On 5/21/2014 6:58 PM, Coleen Phillimore wrote:
>
> I agree.  This is a nice change.
> Coleen
>
> On 5/21/14, 4:54 PM, Vladimir Kozlov wrote:
>> 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
>>>>>>>>
>>>>>>>
>>>>>
>>>
>