Re: [HACKERS] Recently added typedef "string" is a horrid idea

[Tom Lane]

Peter Geoghegan  writes:
> On Sun, Feb 7, 2016 at 7:47 AM, Tom Lane  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

Re: [HACKERS] Recently added typedef "string" is a horrid idea

[Peter Geoghegan]

On Sun, Feb 7, 2016 at 7:47 AM, Tom Lane  wrote:
> Works for me.

Attached patch is what I came up with. It required only minimal
additional changes for consistency.

-- 
Peter Geoghegan
From 01d8cb278cecb995ecc30cda0125d10c98f4d05c Mon Sep 17 00:00:00 2001
From: Peter Geoghegan 
Date: Sun, 7 Feb 2016 19:18:14 -0800
Subject: [PATCH] Rename struct string to VarString

The previous struct name was likely to cause pgindent to do the wrong
thing, since "string" is a fairly common variable name in general.
---
 src/backend/utils/adt/varlena.c | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 5e7536a..ed0a20a 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -40,7 +40,7 @@
 int			bytea_output = BYTEA_OUTPUT_HEX;
 
 typedef struct varlena unknown;
-typedef struct varlena string;
+typedef struct varlena VarString;
 
 typedef struct
 {
@@ -75,7 +75,7 @@ typedef struct
 #ifdef HAVE_LOCALE_T
 	pg_locale_t locale;
 #endif
-} StringSortSupport;
+} VarStringSortSupport;
 
 /*
  * This should be large enough that most strings will fit, but small enough
@@ -89,8 +89,8 @@ typedef struct
 #define PG_GETARG_UNKNOWN_P_COPY(n) DatumGetUnknownPCopy(PG_GETARG_DATUM(n))
 #define PG_RETURN_UNKNOWN_P(x)		PG_RETURN_POINTER(x)
 
-#define DatumGetStringP(X)			((string *) PG_DETOAST_DATUM(X))
-#define DatumGetStringPP(X)			((string *) PG_DETOAST_DATUM_PACKED(X))
+#define DatumGetVarStringP(X)		((VarString *) PG_DETOAST_DATUM(X))
+#define DatumGetVarStringPP(X)		((VarString *) PG_DETOAST_DATUM_PACKED(X))
 
 static int	varstrfastcmp_c(Datum x, Datum y, SortSupport ssup);
 static int bpcharfastcmp_c(Datum x, Datum y, SortSupport ssup);
@@ -1766,7 +1766,7 @@ varstr_sortsupport(SortSupport ssup, Oid collid, bool bpchar)
 {
 	bool		abbreviate = ssup->abbreviate;
 	bool		collate_c = false;
-	StringSortSupport *sss;
+	VarStringSortSupport *sss;
 
 #ifdef HAVE_LOCALE_T
 	pg_locale_t locale = 0;
@@ -1853,7 +1853,7 @@ varstr_sortsupport(SortSupport ssup, Oid collid, bool bpchar)
 	 */
 	if (abbreviate || !collate_c)
 	{
-		sss = palloc(sizeof(StringSortSupport));
+		sss = palloc(sizeof(VarStringSortSupport));
 		sss->buf1 = palloc(TEXTBUFLEN);
 		sss->buflen1 = TEXTBUFLEN;
 		sss->buf2 = palloc(TEXTBUFLEN);
@@ -1909,8 +1909,8 @@ varstr_sortsupport(SortSupport ssup, Oid collid, bool bpchar)
 static int
 varstrfastcmp_c(Datum x, Datum y, SortSupport ssup)
 {
-	string	   *arg1 = DatumGetStringPP(x);
-	string	   *arg2 = DatumGetStringPP(y);
+	VarString  *arg1 = DatumGetVarStringPP(x);
+	VarString  *arg2 = DatumGetVarStringPP(y);
 	char	   *a1p,
 			   *a2p;
 	int			len1,
@@ -1979,10 +1979,10 @@ bpcharfastcmp_c(Datum x, Datum y, SortSupport ssup)
 static int
 varstrfastcmp_locale(Datum x, Datum y, SortSupport ssup)
 {
-	string	   *arg1 = DatumGetStringPP(x);
-	string	   *arg2 = DatumGetStringPP(y);
-	bool		arg1_match;
-	StringSortSupport *sss = (StringSortSupport *) ssup->ssup_extra;
+	VarString	   *arg1 = DatumGetVarStringPP(x);
+	VarString	   *arg2 = DatumGetVarStringPP(y);
+	bool			arg1_match;
+	VarStringSortSupport *sss = (VarStringSortSupport *) ssup->ssup_extra;
 
 	/* working state */
 	char	   *a1p,
@@ -2134,9 +2134,9 @@ varstrcmp_abbrev(Datum x, Datum y, SortSupport ssup)
 static Datum
 varstr_abbrev_convert(Datum original, SortSupport ssup)
 {
-	StringSortSupport *sss = (StringSortSupport *) ssup->ssup_extra;
-	string	   *authoritative = DatumGetStringPP(original);
-	char	   *authoritative_data = VARDATA_ANY(authoritative);
+	VarStringSortSupport *sss = (VarStringSortSupport *) ssup->ssup_extra;
+	VarString	   *authoritative = DatumGetVarStringPP(original);
+	char		   *authoritative_data = VARDATA_ANY(authoritative);
 
 	/* working state */
 	Datum		res;
@@ -2311,7 +2311,7 @@ done:
 static bool
 varstr_abbrev_abort(int memtupcount, SortSupport ssup)
 {
-	StringSortSupport *sss = (StringSortSupport *) ssup->ssup_extra;
+	VarStringSortSupport *sss = (VarStringSortSupport *) ssup->ssup_extra;
 	double		abbrev_distinct,
 key_distinct;
 
-- 
1.9.1


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

Re: [HACKERS] Recently added typedef "string" is a horrid idea

[Robert Haas]

On Sun, Feb 7, 2016 at 8:03 AM, Peter Geoghegan  wrote:
> On Sat, Feb 6, 2016 at 2:11 PM, Tom Lane  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

Re: [HACKERS] Recently added typedef "string" is a horrid idea

[Peter Geoghegan]

On Sat, Feb 6, 2016 at 2:11 PM, Tom Lane  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

Re: [HACKERS] Recently added typedef "string" is a horrid idea

[Tom Lane]

Peter Geoghegan  writes:
> On Sat, Feb 6, 2016 at 2:11 PM, Tom Lane  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

Re: [HACKERS] Recently added typedef "string" is a horrid idea

[Robert Haas]

On Sat, Feb 6, 2016 at 5:11 PM, Tom Lane  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