Thread: Cleanup - adjust the code crossing 80-column window limit

changes look good to me.

one comment: instead of having block variables onexit, in the while
loops in shmem_exit, can we have a single local variable defined at
the start of the shmem_exit function
and reuse them in the while loops? same comment for onexit block
variable in proc_exit_prepare() function.

Patch applies successfully on commit - 4315e8c23b9a897e12fcf91de7bfd734621096bf

make check and make check-world runs are clean.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


On Wed, Jul 1, 2020 at 12:31 PM Amul Sul <sulamul@gmail.com> wrote:
>
> Hi,
>
> Attached patch makes an adjustment to ipc.c code to be in the 80-column window.
>
> Regards,
> Amul
>

On Wed, Jul 1, 2020 at 4:29 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> changes look good to me.

Thanks for looking at the patch.

>
> one comment: instead of having block variables onexit, in the while
> loops in shmem_exit, can we have a single local variable defined at
> the start of the shmem_exit function
> and reuse them in the while loops? same comment for onexit block
> variable in proc_exit_prepare() function.
>

If you are worried about the declaration and initialization of the variable will
happen with every loop cycle then you shouldn't because that happens only
once before the loop-block is entered.

Regards,
Amul

> >
> > one comment: instead of having block variables onexit, in the while
> > loops in shmem_exit, can we have a single local variable defined at
> > the start of the shmem_exit function
> > and reuse them in the while loops? same comment for onexit block
> > variable in proc_exit_prepare() function.
> >
>
> If you are worried about the declaration and initialization of the variable will
> happen with every loop cycle then you shouldn't because that happens only
> once before the loop-block is entered.
>

thanks. understood (just for info [1]) .

Is there a test case covering this part of the code(I'm not sure if
one exists in the regression test suite)?
If no, can we add one?

[1] - https://stackoverflow.com/questions/29785789/why-do-the-objects-created-in-a-loop-have-the-same-address/29785868



With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2020-07-01 09:00, Amul Sul wrote:
> Attached patch makes an adjustment to ipc.c code to be in the 80-column 
> window.

I can see an argument that this makes the code a bit easier to read, but 
making code fit into 80 columns doesn't have to be a goal by itself.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jul 3, 2020 at 1:32 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
>
> On 2020-07-01 09:00, Amul Sul wrote:
> > Attached patch makes an adjustment to ipc.c code to be in the 80-column
> > window.
>
> I can see an argument that this makes the code a bit easier to read, but
> making code fit into 80 columns doesn't have to be a goal by itself.
>
I wouldn't disagree with that. I believe the 80 column rule has been documented
for the code readability.

Regards,
Amul

>
> Is there a test case covering this part of the code(I'm not sure if
> one exists in the regression test suite)?
> If no, can we add one?
>

I observed that the code areas this patch is trying to modify are
pretty much generic and are being called from many places.
The code basically handles exit callbacks on proc exits, on or before
shared memory exits which is very generic and common code.
I'm sure these parts are covered with the existing regression test suites.

Since I have previously run the regression tests, now finally, +1 for
the patch from my end.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com