Re: PATCH: CreateComments: use explicit indexing for ``values''

I have left update_attstat which I'm unsure about, and have attached the updated patch handling the other cases. This will be linked in via the commitfest page.

I picked commands/comment.c randomly and found the i = 0, i++ method of initializing the array made it harder for me to visualize it's structure, or verify each value was in the correct place in the array.

Obviously we can assume each value is in the correct place because it's been like that in working code.

This patch makes the intent of each initialization clear by using the constants directly instead of in a comment, and has the effect of being able to verify each line on it's own. The original requires verification of the preceding i++'s.

I expanded upon my original patch of only handling CreateComments to include the other cases for consistency - but only so far as update_attstats as I found it too complex for me and left it. I don't like to break anything.

I'm not going to push for it if most people prefer the original code for readability, or maintainability across versions; I just thought I'd post my thoughts with a patch.

An easier route, might be for me to submit a patch which just changes comment.c by adding in the constants via a comment like the other places. What do you think ?

Richard

--- On Tue, 14/6/11, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:

> From: Alvaro Herrera <alvherre(at)commandprompt(dot)com>
> Subject: Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
> To: "richhguard-monotone" <richhguard-monotone(at)yahoo(dot)co(dot)uk>
> Cc: "pgsql-hackers" <pgsql-hackers(at)postgresql(dot)org>
> Date: Tuesday, 14 June, 2011, 15:20
> Excerpts from richhguard-monotone's
> message of lun jun 13 16:10:17 -0400 2011:
> > Apologies - I meant to CC in the list but forgot.
> >
> > I have gone through and changed all the related
> functions except ``update_attstats''.
> >
> > Do you have any advice of how to handle the inner
> loops, such as those initializing ``stakindN''. The entries
> before can be handled just like in this patch, by using the
> symbolic constants.
>
> Based on Tom's comments, I'd submit the patch without that
> bit, at least
> as a first step.
>
> > Again, this is based on master and all existing tests
> pass.
>
> Please post the patch and add it here:
> https://commitfest.postgresql.org/action/commitfest_view?id=10
>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development,
> 24x7 support
>

On Jun14, 2011, at 17:47 , richhguard-monotone(at)yahoo(dot)co(dot)uk wrote:
> This patch makes the intent of each initialization clear by using
> the constants directly instead of in a comment, and has the effect
> of being able to verify each line on it's own. The original requires
> verification of the preceding i++'s.

Here's a review of that patch.

The patch applies cleanly to HEAD, looks correct, and passes "make check".

The patch makes the code far more readable and makes adding new columns
to one of the affected system catalogs less error-prone.

Since the chance of us ever back-patching changes to the system catalog's
structure, the patch doesn't introduce additional back-patching hurdles.

I'm thus marking this Read for Committer.

best regards,
Florian Pflug

richhguard-monotone(at)yahoo(dot)co(dot)uk writes:
> This patch makes the intent of each initialization clear by using the constants directly instead of in a comment, and has the effect of being able to verify each line on it's own. The original requires verification of the preceding i++'s.

Applied, along with changing update_attstats() using Alvaro's idea.

regards, tom lane