RFR: 8000975: (process) Merge UNIXProcess.java.bsd & UNIXProcess.java.linux (& .solaris & .aix)

[Peter Levart]

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.Bateman at oracle.com 
> <mailto:Alan.Bateman at 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.
>
>