Thread: indentation in _hash_pgaddtup()

> On 24 Nov 2022, at 13:42, Ted Yu <yuzhihong@gmail.com> wrote:

> In _hash_pgaddtup(), it seems the indentation is off for the assertion.
>
> Please take a look at the patch.

Indentation is handled by applying src/tools/pgindent to the code, and
re-running it on this file yields no re-indentation so this is in fact correct
according to the pgindent rules.

--
Daniel Gustafsson        https://vmware.com/

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 24 Nov 2022, at 13:42, Ted Yu <yuzhihong@gmail.com> wrote:
>> In _hash_pgaddtup(), it seems the indentation is off for the assertion.

> Indentation is handled by applying src/tools/pgindent to the code, and
> re-running it on this file yields no re-indentation so this is in fact correct
> according to the pgindent rules.

It is one messy bit of code though --- perhaps a little more thought
about where to put line breaks would help?  Alternatively, it could
be split into multiple statements, along the lines of

#ifdef USE_ASSERT_CHECKING
    if (PageGetMaxOffsetNumber(page) > 0)
    {
        IndexTuple lasttup = PageGetItem(page,
                                         PageGetItemId(page,
                                                       PageGetMaxOffsetNumber(page)));

        Assert(_hash_get_indextuple_hashkey(lasttup) <=
               _hash_get_indextuple_hashkey(itup));
    }
#endif

(details obviously tweakable)

            regards, tom lane

On Fri, 25 Nov 2022 at 04:29, Ted Yu <yuzhihong@gmail.com> wrote:
> Here is patch v2.

After running pgindent on v2, I see it still pushes the lines out
quite far. If I add a new line after PageGetItemId(page, and put the
variable assignment away from the variable declaration then it looks a
bit better. It's still 1 char over the limit.

David

David Rowley <dgrowleyml@gmail.com> writes:
> After running pgindent on v2, I see it still pushes the lines out
> quite far. If I add a new line after PageGetItemId(page, and put the
> variable assignment away from the variable declaration then it looks a
> bit better. It's still 1 char over the limit.

If you wanted to be hard-nosed about 80 character width, you could
pull out the PageGetItemId call into a separate local variable.
I wasn't going to be quite that picky, but I won't object if that
seems better to you.

            regards, tom lane

On Fri, 25 Nov 2022 at 09:40, Ted Yu <yuzhihong@gmail.com> wrote:
> Patch v4 stores ItemId in a local variable.

ok, I pushed that one.  Thank you for working on this.

David

On Fri, 25 Nov 2022 at 09:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> If you wanted to be hard-nosed about 80 character width, you could
> pull out the PageGetItemId call into a separate local variable.
> I wasn't going to be quite that picky, but I won't object if that
> seems better to you.

I wasn't too worried about the 1 char, but Ted wrote a patch and it
looked a little nicer.

David