RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Daniel D. Daugherty]

On 12/6/19 3:04 AM, Langer, Christoph wrote:
> Thanks, David.
>
> I'll run the final change once again through jdk-submit befor pushing.
>
> Alan, Dan, may I consider this reviewed by either of you?

Sorry I haven't done a full review on this change. I just happened
to see the typo fly by...

Dan


>
> Thanks
> Christoph
>
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Freitag, 6. Dezember 2019 08:02
>> To: Langer, Christoph <christoph.langer at sap.com>;
>> daniel.daugherty at oracle.com
>> Cc: core-libs-dev at openjdk.java.net; hotspot-runtime-
>> dev at openjdk.java.net; Alan Bateman <Alan.Bateman at oracle.com>; gerard
>> ziemski <gerard.ziemski at oracle.com>
>> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function between
>> libjava, hotspot and libinstrument
>>
>> Hi Christoph,
>>
>> On 6/12/2019 2:12 am, Langer, Christoph wrote:
>>> Hi David,
>>>
>>> I prepared an updated webrev:
>> http://cr.openjdk.java.net/~clanger/webrevs/8234185.3/
>>>> src/java.base/windows/native/libjava/canonicalize_md.c
>>>>
>>>> +// We can't include jdk_util.h here because the file is used in Oracle
>>>> in another context
>>>> +// #include "jdk_util.h"
>>>>
>>>> Seems to me clients of JDK_Canonicalize need to include jdk_util.h, not
>>>> the files that implement it. So there is no reason to include it here
>>>> and so no need for the comment.
>>> Well, it's actually not needed but I think it's good practice that the declaring
>> header is included in the implementation file.
>>
>> Yes I was forgetting the importance of ensuring the declaration in the
>> header matches the definition. There is a typo in the comment "possibile".
>>
>>>> Further, does:
>>>>
>>>> src/java.base/unix/native/libjava/canonicalize_md.c
>>>>
>>>> need to include jdk_util.h? I think not.
>>> Same as for the windows file - not necessary but good style.
>>>
>>>> +/* The appropriate location of getPrefixed() is io_util_md.c, but it is
>>>> +   also used in a non-OpenJDK context within Oracle. There,
>>>> canonicalize_md.c
>>>> +   is already pulled in and compiled, so to avoid more complicate solutions
>>>> +   we keep this method here. */
>>>>
>>>> I don't like to have comments like this but I guess it is needed here.
>>>> Please put the */ on a newline. Also s/complicate/complicates/
>>> I did as Dan pointed out.
>> :) Yes sorry about that slip.
>>
>> Otherwise all looks good. No need for new webrev for the typo.
>>
>> Thanks,
>> David
>>
>>>> src/java.base/windows/native/libjava/io_util_md.c
>>>>
>>>> should now be unchanged, but you've left in the copyright update.
>>>>
>>> Right, thanks for the catch.
>>>
>>>> A second review is still needed for the final version of this.
>>> Dan, can I add you as reviewer then?
>>>
>>> Best regards
>>> Christoph
>>>