Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

[Ulf Zibis]

Am 18.04.2011 15:43, schrieb Michael McMahon:
> Ulf Zibis wrote:
>> 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.

Hm, can't share that reason. But anyway, the comment would be little closer to the code, if the 
constant would at least be defined in the mothod's scope.

>> 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.
Not always yes, but each code line has to be understood by the reader. The more lines to read, the 
more lines to understand, apart from the bigger memory and codebase footprint. Just my opinion.

>> 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.
I don't understand. The overhead is bigger while using a List than for a simple array.

> Or if it is, then we should be looking at our List implementations.
If there would be thousands of processes to create, I would agree. HotSpot would optimize to 
equivalent performance, but in real world, I guess, VM's interpreter has to deal with the more 
overloaded code from the List stuff.


-Ulf