boolin comment not moved when code was refactored
Hi.
I happened upon a function comment referring to non-existent code
(that code was moved to another location many years ago).
Probably better to move that comment too. Thoughts?
PSA.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v1-0001-Relocate-old-comment.patchapplication/octet-stream; name=v1-0001-Relocate-old-comment.patchDownload
From 9f97bdbb1f2000d9cfca8d23d31036674a1cbd17 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Thu, 19 Oct 2023 13:23:15 +1100
Subject: [PATCH v1] Relocate old comment
---
src/backend/utils/adt/bool.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/backend/utils/adt/bool.c b/src/backend/utils/adt/bool.c
index cc4bd55..720edff 100644
--- a/src/backend/utils/adt/bool.c
+++ b/src/backend/utils/adt/bool.c
@@ -35,6 +35,7 @@ parse_bool(const char *value, bool *result)
bool
parse_bool_with_len(const char *value, size_t len, bool *result)
{
+ /* Check the most-used possibilities first. */
switch (*value)
{
case 't':
@@ -123,8 +124,6 @@ parse_bool_with_len(const char *value, size_t len, bool *result)
*
* Check explicitly for "true/false" and TRUE/FALSE, 1/0, YES/NO, ON/OFF.
* Reject other values.
- *
- * In the switch statement, check the most-used possibilities first.
*/
Datum
boolin(PG_FUNCTION_ARGS)
--
1.8.3.1
On Thu, Oct 19, 2023 at 10:35 AM Peter Smith <smithpb2250@gmail.com> wrote:
Hi.
I happened upon a function comment referring to non-existent code
(that code was moved to another location many years ago).Probably better to move that comment too. Thoughts?
Agreed. +1 to move that comment.
Thanks
Richard
Richard Guo <guofenglinux@gmail.com> writes:
On Thu, Oct 19, 2023 at 10:35 AM Peter Smith <smithpb2250@gmail.com> wrote:
I happened upon a function comment referring to non-existent code
(that code was moved to another location many years ago).Probably better to move that comment too. Thoughts?
Agreed. +1 to move that comment.
Hm, I'm inclined to think that the comment lines just above:
* boolin - converts "t" or "f" to 1 or 0
*
* Check explicitly for "true/false" and TRUE/FALSE, 1/0, YES/NO, ON/OFF.
* Reject other values.
are also well past their sell-by date. The one-line summary
"converts "t" or "f" to 1 or 0" is not remotely accurate anymore.
Perhaps we should just drop it? Or else reword to something
vaguer, like "input function for boolean". The "Check explicitly"
para no longer describes logic in this function. We could move
it to parse_bool_with_len, but that seems to have a suitable
comment already.
In short, maybe the whole comment should just be
/*
* boolin - input function for type boolean
*/
Agreed with your original point, though.
regards, tom lane
On Thu, Oct 19, 2023 at 2:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Richard Guo <guofenglinux@gmail.com> writes:
On Thu, Oct 19, 2023 at 10:35 AM Peter Smith <smithpb2250@gmail.com> wrote:
I happened upon a function comment referring to non-existent code
(that code was moved to another location many years ago).Probably better to move that comment too. Thoughts?
Agreed. +1 to move that comment.
Hm, I'm inclined to think that the comment lines just above:
* boolin - converts "t" or "f" to 1 or 0
*
* Check explicitly for "true/false" and TRUE/FALSE, 1/0, YES/NO, ON/OFF.
* Reject other values.are also well past their sell-by date. The one-line summary
"converts "t" or "f" to 1 or 0" is not remotely accurate anymore.
Perhaps we should just drop it? Or else reword to something
vaguer, like "input function for boolean". The "Check explicitly"
para no longer describes logic in this function. We could move
it to parse_bool_with_len, but that seems to have a suitable
comment already.
Yes, I had the same thought about the rest of the comment being
outdated but just wanted to test the water to see if a small change
was accepted before I did too much.
In short, maybe the whole comment should just be
/*
* boolin - input function for type boolean
*/
How about "boolin - converts a boolean string value to 1 or 0"
======
Kind Regards,
Peter Smith.
Fujitsu Australia.
On 10/19/23 06:17, Peter Smith wrote:
In short, maybe the whole comment should just be
/*
* boolin - input function for type boolean
*/How about "boolin - converts a boolean string value to 1 or 0"
Personally, I do not like exposing the implementation of a boolean (it
is a base type that is not a numeric), so I prefer Tom's suggestion.
--
Vik Fearing
On Thu, Oct 19, 2023 at 3:26 PM Vik Fearing <vik@postgresfriends.org> wrote:
On 10/19/23 06:17, Peter Smith wrote:
In short, maybe the whole comment should just be
/*
* boolin - input function for type boolean
*/How about "boolin - converts a boolean string value to 1 or 0"
Personally, I do not like exposing the implementation of a boolean (it
is a base type that is not a numeric), so I prefer Tom's suggestion.
OK. Done that way.
PSA v2.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v2-0001-Relocate-or-remove-stale-comments.patchapplication/octet-stream; name=v2-0001-Relocate-or-remove-stale-comments.patchDownload
From b4b4f80892d799cdf78bdb55de89b5a91d65ab3e Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Thu, 19 Oct 2023 16:14:15 +1100
Subject: [PATCH v2] Relocate or remove stale comments
---
src/backend/utils/adt/bool.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/adt/bool.c b/src/backend/utils/adt/bool.c
index cc4bd55..16ae96a 100644
--- a/src/backend/utils/adt/bool.c
+++ b/src/backend/utils/adt/bool.c
@@ -35,6 +35,7 @@ parse_bool(const char *value, bool *result)
bool
parse_bool_with_len(const char *value, size_t len, bool *result)
{
+ /* Check the most-used possibilities first. */
switch (*value)
{
case 't':
@@ -119,12 +120,7 @@ parse_bool_with_len(const char *value, size_t len, bool *result)
*****************************************************************************/
/*
- * boolin - converts "t" or "f" to 1 or 0
- *
- * Check explicitly for "true/false" and TRUE/FALSE, 1/0, YES/NO, ON/OFF.
- * Reject other values.
- *
- * In the switch statement, check the most-used possibilities first.
+ * boolin - input function for type boolean
*/
Datum
boolin(PG_FUNCTION_ARGS)
--
1.8.3.1
Peter Smith <smithpb2250@gmail.com> writes:
PSA v2.
Pushed.
regards, tom lane