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

[Lois Foltan]

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
>>>>>
>>>>
>>