-----BEGIN PGP SIGNED MESSAGE-----
Neil Conway wrote:
| [ Sorry, never saw the original email ]
Because it is still hanging in moderation queue ;)
| FYI, we prefer patches in context diff format (diff -c). Also, there
| are some code style rules that most of the backend code follows. For
I tried to use the same style that was used in the code previously.
Apparently I forgot it in some places.
|>>There were also simple mistakes like in
| That code is long dead, BTW.
Well, we'll se what I can dig out of CVS version :) I think string
handling can be very nasty in some places but the problems are so much
easier to find than with integer overflows.
| I'd agree that StringInfo is appropriate when the string is frequently
| being appended to (and the code using the strlen() pointer arithmetic
| technique you mentioned); however, you've converted the code to use
| StringInfo on situations in which it is clearly not warranted. To pick
| one example at random, seg_atof(char *) in contrib/seg/segparse.y
| doesn't require anything more than a statically sized buffer and
I'm sure I did that, because I really didn't know in all places, what
would be the right thing to do.
Using snprintf() there would cause a log message of "using numeric value
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx..." when trying to overflow this. I
agree, being told number to be unpresentable (coming after errorious
string) is not actually necessary when seeing this :)
| Also, that routine happens to leak memory, since you forgot to call
| pfree(buf.data) -- I believe you made the same mistake in several
| other places, such as seg_yyerror(char *) in the same file.
I checked and this is true. However code leaks already in the same
place. (although less bytes).
| Personally, I prefer this:
| char *buf;
You don't prefer an array of pointers, but I got the point.
| snprintf(buf, sizeof(buf), "...");
| rather than this:
| char *buf;
| snprintf(buf, 1024, "...");
| You used sizeof(...) in some places but not in others.
Very true. I did all my checking and fixing in three nights and didn't
think about the maintainability at first but started using
sizeof(later). I just wanted to get them fixed at first. These should
all be using sizeof(buf) when the target is an array.
There were also places where a simple pointer to a buffer was passed to
another function which then appended some string to it. I think this was
date/time handling in somewhere. That kind of things are impossible to
fix (without changing the function definition) if the appended string
doesn't have a certain maximum size. Dates/times sure have that limit,
but I hope no one copies that code to handle any some variable length
| FYI, running the regression tests is an easy way to do some basic
| testing. Since code that causes regression tests to fail won't be
| accepted (period), you may as well run them now, rather now later.
- - Jukka
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.7 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
-----END PGP SIGNATURE-----
In response to
pgsql-hackers by date
|Next:||From: Neil Conway||Date: 2002-08-29 05:27:41|
|Subject: tweaking MemSet() performance|
|Previous:||From: Bruce Momjian||Date: 2002-08-29 05:03:05|
|Subject: Re: [SQL] LIMIT 1 FOR UPDATE or FOR UPDATE LIMIT 1?|