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 <john.zavg...@oracle.com>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: marti...@google.com > To: chris...@zoulas.com > Cc: john.zavg...@oracle.com, core-libs-dev@openjdk.java.net > 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 <chris...@zoulas.com>wrote: > >> On Mar 21, 11:36am, john.zavg...@oracle.com (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; >
