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 Thu, Nov 24, 2022 at 7:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

Thanks Tom for the suggestion.

Here is patch v2.

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 Thu, Nov 24, 2022 at 12:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

Patch v4 stores ItemId in a local variable.

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