Re: proposal (9.5) : psql unicode border line styles - Mailing list pgsql-hackers

From Tomas Vondra
Subject Re: proposal (9.5) : psql unicode border line styles
Date
Msg-id 53CED596.8080008@fuzzy.cz
Whole thread Raw
In response to Re: proposal (9.5) : psql unicode border line styles  (Pavel Stehule <pavel.stehule@gmail.com>)
Responses Re: proposal (9.5) : psql unicode border line styles  (Pavel Stehule <pavel.stehule@gmail.com>)
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Jaime Casanova
Date:
Subject: Re: Shared Data Structure b/w clients
Next
From: Tom Lane
Date:
Subject: Re: Some bogus results from prairiedog