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

[David Holmes]

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
>