Hi Alan, Martin,
Thanks for reviews. I have prepared another webrev based on your feedback:
http://cr.openjdk.java.net/~plevart/jdk9-dev/UNIXProcess/webrev.08/
Comments inline...
On 04/30/2014 05:17 PM, Alan Bateman wrote:
I reviewed previous rounds and you've addressed my points so I think
I'm mostly happy with this. As some point I think we should look at
Platform again as there may be an opportunity later in JDK 9 to move
it out of UNIXProcess.
One thing that I wasn't sure about is the additions to the @author
tags as we've mostly been trying not to grow these (contentious topic,
I don't have a strong opinion as I never use @author).
I have removed myself, but left all the others (merged from all 4
files). It's true that mercurial keeps track of authors and it's not
hard to find out who's responsible for what part of code in case someone
needs to ask the author about the not so obvious things, but
refactorings like this, which eliminate entire files and move their
content to other places, make this tracking harder.
A minor comment looking at the latest webrev.07 is that some of the
lines are very long. It's not a problem now but I could image future
side-by-side reviews needing a scroll
bar.DeferredCloseProcessPipeInputStream is very long too.
I split the longer lines now. The DeferredCloseProcessPipeInputStream is
admittedly a long name, but it's not public and I tried to express the
fact that it is a class that merges the functionalities of
DeferedCloseInputStream and ProcessPipeInputStream (in .aix file it was
called ProcessPipeInputStream as it was derived from it, but we can't
have two with same names in common file). I hope in further re-factoring
we can merge/consolidate these wrapper streams too and at that time I
think they could be renamed. For now I wanted to keep the functionality
as-is. But I can rename it if so desired, I just can't think of a
suitable name.
On 04/30/2014 06:25 PM, Martin Buchholz wrote:
Belated review.
Peter, thank you very much for taking on this difficult unification
task. This is great work!
Comments:
---
---
+ case SOLARIS:
+ if (osArch.equals("x86")) { osArch = "i386"; }
It's very sad that ... there's no portable way of getting the name of
libarchdir. I thought os.arch system property would suffice, but
apparently not on Solaris?
...and BSD (OS X). Is OS X missing the architecture sub-directories
because it only supports one architecture now or because it used to
employ "fat" (multi-architecture) binaries?
---
To avoid repetition, I would iniitalize with a vararg list of launch
mechanisms, of which the first one is always the default.
+ LINUX(LaunchMechanism.VFORK, EnumSet.of(LaunchMechanism.FORK,
LaunchMechanism.VFORK)),
LINUX(LaunchMechanism.VFORK, LaunchMechanism.FORK),
I've done that. It looks nicer now.
---
Never use toUpperCase without a Locale, to avoid the "Turkish i problem"
+ lm =
LaunchMechanism.valueOf(s.toUpperCase());
toUpperCase(Locale.ENGLISH)
Oops, thanks for spotting this. Fixed.
Regards, Peter
On Wed, Apr 30, 2014 at 8:17 AM, Alan Bateman <alan.bate...@oracle.com
<mailto:alan.bate...@oracle.com>> wrote:
On 25/04/2014 17:47, roger riggs wrote:
Hi Peter,
Including the test update with the updated changeset is fine.
(I think Alan had some comments on the refactoring and has not
yet had a chance to comment).
Thanks, Roger
I reviewed previous rounds and you've addressed my points so I
think I'm mostly happy with this. As some point I think we should
look at Platform again as there may be an opportunity later in JDK
9 to move it out of UNIXProcess.
One thing that I wasn't sure about is the additions to the @author
tags as we've mostly been trying not to grow these (contentious
topic, I don't have a strong opinion as I never use @author).
A minor comment looking at the latest webrev.07 is that some of
the lines are very long. It's not a problem now but I could image
future side-by-side reviews needing a scroll
bar.DeferredCloseProcessPipeInputStream is very long too.
-Alan.
