Re: [PATCH] Add min() and max() aggregate functions for xid8

On 2022/02/08 18:43, Ken Kato wrote:
> On 2022-02-08 15:34, Fujii Masao wrote:
>> Thanks for updating the patch! It basically looks good to me. I
>> applied the following small changes to the patch. Updated version of
>> the patch attached. Could you review this version?
>
> Thank you for the patch!
>
> It looks good to me!

Thanks for the review!

> I'm not sure how strict coding conventions are, but the following line is over 80 characters.
> +insert into xid8_t1 values ('0'), ('010'), ('42'), ('0xffffffffffffffff'), ('-1');
> Therefore, I made a patch which removed ('010') just to fit in 80 characters.

If you want to avoid the line longer than 80 columns, you should break it into two or more rather than remove the test code, I think. What to test is more important than formatting.

Also the following descriptions about formatting would be helpful.

---------------------------
https://www.postgresql.org/docs/devel/source-format.html

Limit line lengths so that the code is readable in an 80-column window.
(This doesn't mean that you must never go past 80 columns. For instance,
breaking a long error message string in arbitrary places just to keep
the code within 80 columns is probably not a net gain in readability.)
---------------------------

Therefore I'm ok with the patch that I posted upthread. Also I'm ok if you will break that longer line into two and post new patch. Or if the value '010' is really useless for the test purpose, I'm also ok if you remove it. Thought?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION