Review request for 7034570 - java.lang.Runtime.exec(String cmd, String env) can not work properly if SystemRoot not inherited
michael.x.mcmahon at oracle.com
Mon Apr 18 13:43:49 UTC 2011
Ulf Zibis wrote:
> Am 16.04.2011 12:48, schrieb Alan Bateman:
>> Michael McMahon wrote:
>>> I have incorporated much of Ulf's approach into this version.
>>> I agree with Alan about the spec. We could find other variables in
>>> the future that need
>>> to be set, but can have alternative values other than the default. I
>>> think this approach
>>> is the most flexible.
>> This looks much better - thanks Ulf for the improved loop and
>> changing it to use nameComparator. It will be good to get this one
> Thanks for the partial acceptance.
> I think, the comment in lines 295..297 more belongs to the code (lines
> 312..315 + 318..320 ) rather than to the constant SYSTEMROOT. Why
> defining it as class member, as it is only locally referred?
It makes no difference to the generated code. And imo, there is more
space for the comment there.
> I still don't see the advantage, having the SYSTEMROOT insertion code
> at 2 call sites. One would be shorter and much clearer as shown in my
> code. Then we too could save addToEnvIfSet(..) and the call to it.
Shorter doesn't always mean clearer. I think this way is clearer.
> Additionally: Why instantiating, initializing and processing a fat
> j.u.List object, if a simple array would fulfil all needs?
I don't see a compelling reason to change it from a List (which is what
it was). I think any additional overhead
is inconsequential in the context of creating an OS process. Or if it
is, then we should be looking at
our List implementations.
> You have removed the 'Ensure double NUL termination' code. I guess,
> you have approved, that String->CString already handles this, right?
Actually no. That shouldn't have been omitted. Thanks for finding that!
More information about the core-libs-dev