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