Bracket, brace, parenthesis

Started by Kyotaro Horiguchiover 4 years ago7 messages

horikyota.ntt@gmail.com

over 4 years ago

1 attachment(s)

I found the following code in multirangetypes.c

if (*ptr == '{')
ptr++;
else
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("malformed multirange literal: \"%s\"",
input_str),
errdetail("Missing left bracket.")));

I'm not sure how much we (or people) are strcit on the distinction
between the $SUBJECT, isn't '{' a brace generally?

postgres=# select '[1,3]'::int4multirange;
ERROR: malformed multirange literal: "[1,3]"
LINE 1: select '[1,3]'::int4multirange;
^
DETAIL: Missing left bracket.

The distinction is significant there. It should at least be "Missing
left curly bracket." or "Missing left brace." (or left curly brace..?)

'{' is mentioned as "curly brackets" in comments of the soruce file.
It is mentioned as "brace" in regexp doc [1]https://www.postgresql.org/docs/devel/functions-matching.html. And.. uh.. I found the
world "curly braces" in the doc for conding conventions..

[1]: https://www.postgresql.org/docs/devel/functions-matching.html

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Attachments:

brace_not_bracket.patchtext/x-patch; charset=us-asciiDownload

diff --git a/src/backend/utils/adt/multirangetypes.c b/src/backend/utils/adt/multirangetypes.c
index 0b81649779..fbcc27d072 100644
--- a/src/backend/utils/adt/multirangetypes.c
+++ b/src/backend/utils/adt/multirangetypes.c
@@ -146,7 +146,7 @@ multirange_in(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
 				 errmsg("malformed multirange literal: \"%s\"",
 						input_str),
-				 errdetail("Missing left bracket.")));
+				 errdetail("Missing left brace.")));
 
 	/* consume ranges */
 	parse_state = MULTIRANGE_BEFORE_RANGE;
@@ -282,7 +282,7 @@ multirange_in(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
 				 errmsg("malformed multirange literal: \"%s\"",
 						input_str),
-				 errdetail("Junk after right bracket.")));
+				 errdetail("Junk after right brace.")));
 
 	ret = make_multirange(mltrngtypoid, rangetyp, range_count, ranges);
 	PG_RETURN_MULTIRANGE_P(ret);

Tom Lane

tgl@sss.pgh.pa.us

over 4 years ago

In reply to: Kyotaro Horiguchi (#1)

Re: Bracket, brace, parenthesis

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:

I'm not sure how much we (or people) are strcit on the distinction
between the $SUBJECT, isn't '{' a brace generally?

+1. I tend to write "square bracket" or "curly brace" when I want to
be extra clear, but I think the bare terms are widely understood to
have those meanings.

regards, tom lane

Kyotaro Horiguchi

horikyota.ntt@gmail.com

over 4 years ago

In reply to: Tom Lane (#2)

1 attachment(s)

Re: Bracket, brace, parenthesis

At Fri, 14 May 2021 10:04:57 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:

I'm not sure how much we (or people) are strcit on the distinction
between the $SUBJECT, isn't '{' a brace generally?

+1. I tend to write "square bracket" or "curly brace" when I want to
be extra clear, but I think the bare terms are widely understood to
have those meanings.

Thanks! I think the message is new in 14 so we can fix it right
away. The attached is the version with a commit message added.

If not, I'll register this to the next CF.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Attachments:

0001-Change-confusing-bracket-use-to-clearer-wording.patchtext/x-patch; charset=us-asciiDownload

From e0794b27583d5cbc50c59497343d77171a169f17 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 27 May 2021 15:01:44 +0900
Subject: [PATCH] Change confusing 'bracket' use to clearer wording

The current error message looks like this.

LINE 1: select '[1,3]'::int4multirange;
               ^
DETAIL:  Missing left bracket.

It is quite confusing when mentioning a string that contains both of
brackets"[]" and braces"{}".

We are using several kind of wordings point "{}" but the bare word
"brace" is clear enough here.
---
 src/backend/utils/adt/multirangetypes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/adt/multirangetypes.c b/src/backend/utils/adt/multirangetypes.c
index 0b81649779..fbcc27d072 100644
--- a/src/backend/utils/adt/multirangetypes.c
+++ b/src/backend/utils/adt/multirangetypes.c
@@ -146,7 +146,7 @@ multirange_in(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
 				 errmsg("malformed multirange literal: \"%s\"",
 						input_str),
-				 errdetail("Missing left bracket.")));
+				 errdetail("Missing left brace.")));
 
 	/* consume ranges */
 	parse_state = MULTIRANGE_BEFORE_RANGE;
@@ -282,7 +282,7 @@ multirange_in(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
 				 errmsg("malformed multirange literal: \"%s\"",
 						input_str),
-				 errdetail("Junk after right bracket.")));
+				 errdetail("Junk after right brace.")));
 
 	ret = make_multirange(mltrngtypoid, rangetyp, range_count, ranges);
 	PG_RETURN_MULTIRANGE_P(ret);
-- 
2.27.0

Michael Paquier

michael@paquier.xyz

over 4 years ago

In reply to: Kyotaro Horiguchi (#3)

Re: Bracket, brace, parenthesis

On Thu, May 27, 2021 at 03:20:10PM +0900, Kyotaro Horiguchi wrote:

At Fri, 14 May 2021 10:04:57 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in

+1. I tend to write "square bracket" or "curly brace" when I want to
be extra clear, but I think the bare terms are widely understood to
have those meanings.

Thanks! I think the message is new in 14 so we can fix it right
away. The attached is the version with a commit message added.

No objections from me to fix that on HEAD now for clarity, let's wait
a bit and see if others have more comments. You have missed an update
of multirangetypes.out, by the way.
--
Michael

Kyotaro Horiguchi

horikyota.ntt@gmail.com

over 4 years ago

In reply to: Michael Paquier (#4)

2 attachment(s)

Re: Bracket, brace, parenthesis

At Thu, 27 May 2021 21:08:46 +0900, Michael Paquier <michael@paquier.xyz> wrote in

On Thu, May 27, 2021 at 03:20:10PM +0900, Kyotaro Horiguchi wrote:

At Fri, 14 May 2021 10:04:57 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in

+1. I tend to write "square bracket" or "curly brace" when I want to
be extra clear, but I think the bare terms are widely understood to
have those meanings.

Thanks! I think the message is new in 14 so we can fix it right
away. The attached is the version with a commit message added.

No objections from me to fix that on HEAD now for clarity, let's wait
a bit and see if others have more comments. You have missed an update
of multirangetypes.out, by the way.

Mmm. Thanks. So the test doesn't a check for the case of trailing
garbage. Looking the discussion about trailing garbage of interger
values, we might need one for the case.

The atached second file adds a test for trailing garbage for
multirangetype.sql and rangetype.sql.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Attachments:

v2-0001-Change-confusing-bracket-use-to-clearer-wording.patchtext/x-patch; charset=us-asciiDownload

From 3c503f9a7982bc583b84876a3019a4a3a28209ca Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 27 May 2021 15:01:44 +0900
Subject: [PATCH v2 1/2] Change confusing 'bracket' use to clearer wording

The current error message looks like this.

LINE 1: select '[1,3]'::int4multirange;
               ^
DETAIL:  Missing left bracket.

It is quite confusing when mentioning a string that contains both of
brackets"[]" and braces"{}".

We are using several kind of wordings point "{}" but the bare word
"brace" is clear enough here.
---
 src/backend/utils/adt/multirangetypes.c       | 4 ++--
 src/test/regress/expected/multirangetypes.out | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/backend/utils/adt/multirangetypes.c b/src/backend/utils/adt/multirangetypes.c
index 0b81649779..fbcc27d072 100644
--- a/src/backend/utils/adt/multirangetypes.c
+++ b/src/backend/utils/adt/multirangetypes.c
@@ -146,7 +146,7 @@ multirange_in(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
 				 errmsg("malformed multirange literal: \"%s\"",
 						input_str),
-				 errdetail("Missing left bracket.")));
+				 errdetail("Missing left brace.")));
 
 	/* consume ranges */
 	parse_state = MULTIRANGE_BEFORE_RANGE;
@@ -282,7 +282,7 @@ multirange_in(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
 				 errmsg("malformed multirange literal: \"%s\"",
 						input_str),
-				 errdetail("Junk after right bracket.")));
+				 errdetail("Junk after right brace.")));
 
 	ret = make_multirange(mltrngtypoid, rangetyp, range_count, ranges);
 	PG_RETURN_MULTIRANGE_P(ret);
diff --git a/src/test/regress/expected/multirangetypes.out b/src/test/regress/expected/multirangetypes.out
index 04953a5990..50f7c37b88 100644
--- a/src/test/regress/expected/multirangetypes.out
+++ b/src/test/regress/expected/multirangetypes.out
@@ -7,7 +7,7 @@ select ''::textmultirange;
 ERROR:  malformed multirange literal: ""
 LINE 1: select ''::textmultirange;
                ^
-DETAIL:  Missing left bracket.
+DETAIL:  Missing left brace.
 select '{,}'::textmultirange;
 ERROR:  malformed multirange literal: "{,}"
 LINE 1: select '{,}'::textmultirange;
-- 
2.27.0

v2-0002-Add-test-cases-for-trailing-garbage-of-multi-rang.patchtext/x-patch; charset=us-asciiDownload

From a04bb0e2cafec4d6cd254ff9d745795a438d811c Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Fri, 28 May 2021 15:21:45 +0900
Subject: [PATCH v2 2/2] Add test cases for trailing garbage of (multi)range
 types.

---
 src/test/regress/expected/multirangetypes.out | 5 +++++
 src/test/regress/expected/rangetypes.out      | 5 +++++
 src/test/regress/sql/multirangetypes.sql      | 1 +
 src/test/regress/sql/rangetypes.sql           | 1 +
 4 files changed, 12 insertions(+)

diff --git a/src/test/regress/expected/multirangetypes.out b/src/test/regress/expected/multirangetypes.out
index 50f7c37b88..98ac592127 100644
--- a/src/test/regress/expected/multirangetypes.out
+++ b/src/test/regress/expected/multirangetypes.out
@@ -13,6 +13,11 @@ ERROR:  malformed multirange literal: "{,}"
 LINE 1: select '{,}'::textmultirange;
                ^
 DETAIL:  Expected range start.
+select '{(,)}.'::textmultirange;
+ERROR:  malformed multirange literal: "{(,)}."
+LINE 1: select '{(,)}.'::textmultirange;
+               ^
+DETAIL:  Junk after right brace.
 select '{[a,c),}'::textmultirange;
 ERROR:  malformed multirange literal: "{[a,c),}"
 LINE 1: select '{[a,c),}'::textmultirange;
diff --git a/src/test/regress/expected/rangetypes.out b/src/test/regress/expected/rangetypes.out
index e6ca99d43d..f6170db53a 100644
--- a/src/test/regress/expected/rangetypes.out
+++ b/src/test/regress/expected/rangetypes.out
@@ -9,6 +9,11 @@ ERROR:  malformed range literal: ""
 LINE 1: select ''::textrange;
                ^
 DETAIL:  Missing left parenthesis or bracket.
+select '(,).'::textrange;
+ERROR:  malformed range literal: "(,)."
+LINE 1: select '(,).'::textrange;
+               ^
+DETAIL:  Junk after right parenthesis or bracket.
 select '-[a,z)'::textrange;
 ERROR:  malformed range literal: "-[a,z)"
 LINE 1: select '-[a,z)'::textrange;
diff --git a/src/test/regress/sql/multirangetypes.sql b/src/test/regress/sql/multirangetypes.sql
index 692f2416d9..3cbebedcd4 100644
--- a/src/test/regress/sql/multirangetypes.sql
+++ b/src/test/regress/sql/multirangetypes.sql
@@ -7,6 +7,7 @@
 -- negative tests; should fail
 select ''::textmultirange;
 select '{,}'::textmultirange;
+select '{(,)}.'::textmultirange;
 select '{[a,c),}'::textmultirange;
 select '{,[a,c)}'::textmultirange;
 select '{-[a,z)}'::textmultirange;
diff --git a/src/test/regress/sql/rangetypes.sql b/src/test/regress/sql/rangetypes.sql
index cb5353de6f..3a3e1029e0 100644
--- a/src/test/regress/sql/rangetypes.sql
+++ b/src/test/regress/sql/rangetypes.sql
@@ -8,6 +8,7 @@ create type textrange as range (subtype=text, collation="C");
 
 -- negative tests; should fail
 select ''::textrange;
+select '(,).'::textrange;
 select '-[a,z)'::textrange;
 select '[a,z) - '::textrange;
 select '(",a)'::textrange;
-- 
2.27.0

Michael Paquier

michael@paquier.xyz

over 4 years ago

In reply to: Kyotaro Horiguchi (#5)

Re: Bracket, brace, parenthesis

On Fri, May 28, 2021 at 03:25:40PM +0900, Kyotaro Horiguchi wrote:

Mmm. Thanks. So the test doesn't a check for the case of trailing
garbage. Looking the discussion about trailing garbage of integer
values, we might need one for the case.

The atached second file adds a test for trailing garbage for
multirangetype.sql and rangetype.sql.

True for the lack of coverage with some junk after the right brace for
multi-ranges, but rangetypes.sql has already some coverage. Applied
with this small update.
--
Michael

Kyotaro Horiguchi

horikyota.ntt@gmail.com

over 4 years ago

In reply to: Michael Paquier (#6)

Re: Bracket, brace, parenthesis

At Mon, 31 May 2021 11:36:23 +0900, Michael Paquier <michael@paquier.xyz> wrote in

On Fri, May 28, 2021 at 03:25:40PM +0900, Kyotaro Horiguchi wrote:

Mmm. Thanks. So the test doesn't a check for the case of trailing
garbage. Looking the discussion about trailing garbage of integer
values, we might need one for the case.

The atached second file adds a test for trailing garbage for
multirangetype.sql and rangetype.sql.

True for the lack of coverage with some junk after the right brace for
multi-ranges, but rangetypes.sql has already some coverage. Applied
with this small update.

Hmm. Right. Thanks for the check and commiting!

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center