On 12/6/19 3:04 AM, Langer, Christoph wrote:
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?
Sorry I haven't done a full review on this change. I just happened
to see the typo fly by...
Dan
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
