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? Thanks Christoph > -----Original Message----- > From: David Holmes <david.hol...@oracle.com> > Sent: Freitag, 6. Dezember 2019 08:02 > To: Langer, Christoph <christoph.lan...@sap.com>; > daniel.daughe...@oracle.com > Cc: core-libs-dev@openjdk.java.net; hotspot-runtime- > d...@openjdk.java.net; Alan Bateman <alan.bate...@oracle.com>; gerard > ziemski <gerard.ziem...@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 > >
