On 28.6.2014 21:29, Pavel Stehule wrote:
> Hello
> 
> rebase for 9.5
> 
> test:
> \pset linestyle unicode \pset border 2
> \pset unicode_header_linestyle double
> 
> \l
> 
> Regards
> 
> Pavel

I did a quick review of the patch today:

* it applies cleanly to current HEAD (no failures, small offsets)
* compiles and generally seems to work just fine

Two questions:

(1) Shouldn't the new options be listed in '\?' (as possible names for
"pset")? I mean, here:

  \pset [NAME [VALUE]]     set table output option
                         (NAME :=
{format|border|expanded|fieldsep|fieldsep_zero|footer|null|
numericlocale|recordsep|recordsep_zero|tuples_only|title|tableattr|pager})


(2) I noticed this piece of code:

+typedef enum unicode_linestyle
+{
+       UNICODE_LINESTYLE_SINGLE = 0, /* to make sure someone initializes this 
*/
+       UNICODE_LINESTYLE_DOUBLE = 1
+} unicode_linestyle;

Why are the values defined explicitly? These values are set by the
compiled automatically, so why set them manually? Only a few of the
other enums are defined explicitly, and most of them have to do that to
define different values (e.g. 0x01, 0x02, 0x04, ...).

I don't understand how the comment "to make sure someone initializes
this" explains the purpose?


regards
Tomas


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to