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

[Langer, Christoph]

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