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

Langer, Christoph christoph.langer at sap.com
Thu Dec 5 16:12:06 UTC 2019


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.

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

> 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



More information about the core-libs-dev mailing list