Recently added typedef "string" is a horrid idea
I see that commit b47b4dbf6 added this to varlena.c:
typedef struct varlena string;
This is a remarkably bad idea. It will cause pgindent to do strange
things anywhere it sees a variable or field named "string", of which
we have quite a few. Remember that the effects of typedef names are
*global*, so far as pgindent is concerned; not only varlena.c will
be affected.
Please rename this typedef with some less-generic name. Probably
some of the other identifiers added in the same commit should be
adjusted to match.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Feb 6, 2016 at 5:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I see that commit b47b4dbf6 added this to varlena.c:
typedef struct varlena string;
This is a remarkably bad idea. It will cause pgindent to do strange
things anywhere it sees a variable or field named "string", of which
we have quite a few. Remember that the effects of typedef names are
*global*, so far as pgindent is concerned; not only varlena.c will
be affected.Please rename this typedef with some less-generic name. Probably
some of the other identifiers added in the same commit should be
adjusted to match.
Oops. I didn't foresee that outcome. I'm not sure offhand what else
to call it, but I suppose we can come up with something.
"charactertype", maybe?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Feb 6, 2016 at 2:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Remember that the effects of typedef names are
*global*, so far as pgindent is concerned; not only varlena.c will
be affected.
I'll remember that in the future.
Please rename this typedef with some less-generic name. Probably
some of the other identifiers added in the same commit should be
adjusted to match.
I suggest "VarString". All the text SortSupport routines were renamed
to match a pattern of "varstr.*" as part of the commit you mention.
The implication that was intended by the rename is that the relevant
routines are responsible for about the same cases as the cases handled
by varstr_cmp(). I tend to mostly think of the text type when looking
at varstr_cmp(), but it's also used by jsonb, for example, as well as
char(n). It has a broader purpose; it is used by collatable types
generally. So, a rename to "VarString" probably makes sense,
independent of your pgindent concern.
If this sounds like a good idea, I'll produce a patch soon.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Feb 7, 2016 at 8:03 AM, Peter Geoghegan <pg@heroku.com> wrote:
On Sat, Feb 6, 2016 at 2:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Remember that the effects of typedef names are
*global*, so far as pgindent is concerned; not only varlena.c will
be affected.I'll remember that in the future.
Please rename this typedef with some less-generic name. Probably
some of the other identifiers added in the same commit should be
adjusted to match.I suggest "VarString". All the text SortSupport routines were renamed
to match a pattern of "varstr.*" as part of the commit you mention.The implication that was intended by the rename is that the relevant
routines are responsible for about the same cases as the cases handled
by varstr_cmp(). I tend to mostly think of the text type when looking
at varstr_cmp(), but it's also used by jsonb, for example, as well as
char(n). It has a broader purpose; it is used by collatable types
generally. So, a rename to "VarString" probably makes sense,
independent of your pgindent concern.If this sounds like a good idea, I'll produce a patch soon.
VarString is OK with me - I'm not personally wedded to any specific
proposal here.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Geoghegan <pg@heroku.com> writes:
On Sat, Feb 6, 2016 at 2:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Please rename this typedef with some less-generic name. Probably
some of the other identifiers added in the same commit should be
adjusted to match.
I suggest "VarString". All the text SortSupport routines were renamed
to match a pattern of "varstr.*" as part of the commit you mention.
Works for me.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Feb 7, 2016 at 7:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Works for me.
Attached patch is what I came up with. It required only minimal
additional changes for consistency.
--
Peter Geoghegan
Attachments:
0001-Rename-struct-string-to-VarString.patchtext/x-patch; charset=US-ASCII; name=0001-Rename-struct-string-to-VarString.patchDownload+16-17
Peter Geoghegan <pg@heroku.com> writes:
On Sun, Feb 7, 2016 at 7:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Works for me.
Attached patch is what I came up with. It required only minimal
additional changes for consistency.
I'd already run into some trouble with pgindent messing up on "string",
so I went ahead and pushed this. Thanks!
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers