Hi Jérémie, Thanks for your comments! I'll have a look a it later this week but I've already learnt something: I'm a very bad developer! ^_^' The good news is: it can only get better! :-D
Don't spend time trying to test it. I'll first fix this horrible mess! Cheers, Tanguy 2012/1/22 Jérémie Koenig <[email protected]> > Hi, thanks for working on this. > > A couple of issues I found in your patch: > > > 3) Fix... > > a) "piece of cake" (TM), but include <math.h> (for log10() function) to > get > > an accurate length for the buffer... which may be an overhead! > > It is indeed much trouble for very few bytes saved :-) > > The usual trick for "%d" is to use the constant "sizeof (int) * 3 + > 1". I included + 1 for the sign, but it's not really necessary if we > exepect sizeof(int) >= 2, which we probably should. > > On the other hand, don't forget to include the byte for the > terminating '\0' in your buffer length calculations! > > > b) read file lines in a dynamically re-sized buffer > > The usual approach is to reallocate the buffer geometrically, for instance: > > size = size ? size * 2 : BUFSIZ; > > This way the number of reallocations grows logarithmically rather than > linearily with the actual size we need. > > Also, I think there are some problems with your pointer arithmetic. > For instance, > > if (fgets(buf+last,size,f) == NULL) > > will overwrite the last character read from the previous iteration (if > there was one). > > In fact, > - at entry, "last" points to the first unused byte in your buffer (0), > - on subsequent iterations, it points the last used byte (strlen(buf) - > 1). > > I would say the "first unused byte" convention is more common and > straightforward. > > Also, in: > > if (buf[last+1] != '\n') { return buf; } > > buf[last+1] will always be '\0' (the terminating null character of the > string). I also think you mean to return when the last character _is_ > a newline (fortunately the two mistakes cancel each other out in the > usual case of no reallocation needed :-). > > > c) use lstat as described in readlink man page > > In the error case ("Cannot read link information"), > - "filename" is used after it has been freed > - the two if() blocks could be merged as : > if (len < 0 || len > sb.st_size) { ... } > > > 5) But... > > - memstat uses /proc/XYZ/exe and /proc/XYZ/maps... and the output of > > memstat on Hurd is different than the one on Linux > > I can't test your patch right now but I'd say this is to be expected. > > > - implement (copy/paste mostly) a get_line function. Might have been > better > > to use the GNU get_line()... or not?! > > The getline() function has only recently been added to POSIX, so > portability might be a problem in the short term. What you can use > generally depends on the intended target platforms for the original > code. > > Last, but not least, you should be careful with whitespace. Your patch > mixes tabs and spaces for indentation, and gratuitously removes an > empty line (line 181 in the new version). This may seem trivial at > first, but it's very important for source code to be uniformly > indented, and upstream will rightfully insist that the patches you > submit respect the conventions of the original code. > > > Feel free to make comments on everything (code, decisions, style), I know > > that I still have a lot to learn! > > Don't we all? :-) > > Happy hacking, > -- > Jérémie Koenig <[email protected]> > http://jk.fr.eu.org/ >
