Обсуждение: 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

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

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

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