Thread: resetStringInfo
Attached is a patch that makes a minor addition to the StringInfo API: resetStringInfo(), which clears the current content of the StringInfo but leaves it valid for future operations. I needed this for an external project, but ISTM this would be worth including in mainline: I'd imagine it's a fairly common operation, and there's no easy way to do it at the moment. Clients can manually reset the StringInfo's fields, but that is poor interface design, as well as being error-prone: many people might just reset "len" and forget to NUL-terminate the data buffer and reset the cursor. initStringInfo() could be used, but it reallocates the StringInfo's data buffer, which is often undesirable. -Neil
Attachment
Are there any places in our code where we could use it? If so, please add that to the patch. --------------------------------------------------------------------------- Neil Conway wrote: > Attached is a patch that makes a minor addition to the StringInfo API: > resetStringInfo(), which clears the current content of the StringInfo > but leaves it valid for future operations. > > I needed this for an external project, but ISTM this would be worth > including in mainline: I'd imagine it's a fairly common operation, and > there's no easy way to do it at the moment. Clients can manually reset > the StringInfo's fields, but that is poor interface design, as well as > being error-prone: many people might just reset "len" and forget to > NUL-terminate the data buffer and reset the cursor. initStringInfo() > could be used, but it reallocates the StringInfo's data buffer, which is > often undesirable. > > -Neil > [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 5: don't forget to increase your free space map settings -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Sat, 2007-03-03 at 11:23 -0500, Bruce Momjian wrote: > Are there any places in our code where we could use it? I manually grep'ed around and found a few places where resetStringInfo can be used, but I probably didn't find them all: it's quite hard to find all the places in which "StringInfo->len = 0" is assigned to, given that the StringInfo might have any variable name, and variables like "len" and "buf" are very common for other purposes. Attached is a revised patch, I'll commit it later today. -Neil
Attachment
Neil Conway <neilc@samurai.com> writes:
> Attached is a patch that makes a minor addition to the StringInfo API:
> resetStringInfo(), which clears the current content of the StringInfo
> but leaves it valid for future operations.
> I needed this for an external project, but ISTM this would be worth
> including in mainline:
Sure. I'm pretty sure there are a number of places currently doing this
"by hand"; would you mind looking around to see if you can fix 'em up
to use this function?
regards, tom lane
Neil Conway wrote: > On Sat, 2007-03-03 at 11:23 -0500, Bruce Momjian wrote: > > Are there any places in our code where we could use it? > > I manually grep'ed around and found a few places where resetStringInfo > can be used, but I probably didn't find them all: it's quite hard to > find all the places in which "StringInfo->len = 0" is assigned to, given > that the StringInfo might have any variable name, and variables like > "len" and "buf" are very common for other purposes. I think this is the reason why struct members are prefixed with some short form of the struct name; for example struct timeval has tv_sec and tv_usec. This is used in a lot of places in our code. Maybe it would be a good idea to make it a coding guideline. For example, I'd rename StringInfo members as si_len, si_buf, si_data, which would make this job quite a lot easier. (Yes, the patch would be big, but it's easy to do, if a bit tedious because you just rename the struct and the obvious places, and then let the compiler catch the rest.) -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Tom Lane wrote: > Neil Conway <neilc@samurai.com> writes: >> Attached is a patch that makes a minor addition to the StringInfo API: >> resetStringInfo(), which clears the current content of the StringInfo >> but leaves it valid for future operations. > >> I needed this for an external project, but ISTM this would be worth >> including in mainline: > > Sure. I'm pretty sure there are a number of places currently doing this > "by hand"; would you mind looking around to see if you can fix 'em up > to use this function? I have used pfree(var.data) combined with initStringInfo(&var) in a few places (e.g. in tablefunc.c). Joe
On Sat, 2007-03-03 at 10:57 -0800, Joe Conway wrote: > I have used pfree(var.data) combined with initStringInfo(&var) in a few > places (e.g. in tablefunc.c). Thanks, fixed. I also did a bit more searching and found some more places where resetStringInfo() could be used to replace the previous custom coding. Patch attached and applied. -Neil
Attachment
On Sat, 2007-03-03 at 15:04 -0300, Alvaro Herrera wrote: > I think this is the reason why struct members are prefixed with some > short form of the struct name; for example struct timeval has tv_sec and > tv_usec. This is used in a lot of places in our code. Maybe it would > be a good idea to make it a coding guideline. For example, I'd rename > StringInfo members as si_len, si_buf, si_data, which would make this job > quite a lot easier. I'm not opposed to this idea in general, but I didn't bother doing it for StringInfo as part of this patch. Besides it being more work :), I also wonder if it's a net win to make the change for structs that are directly accessed by code that lives outside the PG tree (e.g. StringInfo). As for making it a coding guideline, I think it would be a good idea to codify some of the more common rules and idioms in the Postgres source tree. Most of these sorts of issues are flagged during code review, but it might save some time in the long run to enumerate some code style suggestions on the wiki or somewhere similar. -Neil