8008118 does not appear to be public - could we fix that? Here is my alternative set of perfectionist changes:
# Cleanup declaration of "environ" http://cr.openjdk.java.net/~martin/webrevs/openjdk8/environ # Fix PATH handling http://cr.openjdk.java.net/~martin/webrevs/openjdk8/pathv/ that could be rolled into one. I adopted Christos' suggestion of using continue; This version only does two allocations. Martin On Mon, Mar 25, 2013 at 2:30 PM, Martin Buchholz <[email protected]>wrote: > Since we're all on this perfectionist quest for quality here, I noticed > that we could replace allocation for path components with a single > allocation using NUL as a separator. I think I'll go code that up. > > Also note to all: if java VMs are created and destroyed without the > process terminating, there is a small leak here (and in many other places > in the JDK). There would be a huge amount of work if we ever wanted to get > that right (especially if we supported concurrently active JVMs). > > > On Fri, Mar 22, 2013 at 7:38 AM, John Zavgren <[email protected]>wrote: > >> Greetings: >> >> I made modifications as per Martin's suggestions: >> 1.) free pathv on "abort". >> 2.) allocate memory for storing the "cwd" string, and then copy it into >> the memory location (to make sure that the contents of the pathv[] array >> don't refer to memory that's from the stack of the procedure that created >> it.) >> >> Thanks! >> John >> >> >> http://cr.openjdk.java.net/~jzavgren/8008118/webrev.06/ >> >> ----- Original Message ----- >> From: [email protected] >> To: [email protected] >> Cc: [email protected], [email protected] >> Sent: Friday, March 22, 2013 10:19:24 AM GMT -05:00 US/Canada Eastern >> Subject: Re: RFR-8008118 >> >> >> >> >> On Thu, Mar 21, 2013 at 12:11 PM, Christos Zoulas <[email protected]>wrote: >> >>> On Mar 21, 11:36am, [email protected] (John Zavgren) wrote: >>> -- Subject: Re: RFR-8008118 >>> >>> | All: >>> | >>> | How does this look? >>> | 1.) I reverted the for statement formatting change. >>> >>> Not exactly. Now it is indented incorrectly. >>> >>> >> Agreed. Really revert. >> >> >>> | 2.) I removed the goto statement and "inlined" some code instead. >>> >>> I prefer to write simpler code that works with both signed and unsigned >>> indexes: >>> >>> + while (i > 0) >>> + if (pathv[--i] != cwd) >>> + free(pathv[i]); >>> + >>> >>> >> Christos' suggestion looks pretty good. >> >> As Mark noted, need to add missing free of pathv itself. >> >> >>> | 3.) I checked to make sure that we're not freeing memory that we >>> didn't act= >>> | ually allocate. (Path vector elements that are empty.) >>> >>> You are still using the "./" string literal constant in the code so >>> you'll >>> end up freeing it (the compiler will prolly merge the two instances and >>> you'll get lucky but it is semantically incorrect). >>> >> >> Agreed, >> >> pathv[i] = cwd; >> > >
