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

Martin Buchholz martinrb at google.com
Wed Apr 30 16:25:06 UTC 2014


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?

---
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),
---
Never use toUpperCase without a Locale, to avoid the "Turkish i problem"
+                            lm = LaunchMechanism.valueOf(s.toUpperCase());
toUpperCase(Locale.ENGLISH)




On Wed, Apr 30, 2014 at 8:17 AM, Alan Bateman <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.
>



More information about the core-libs-dev mailing list