defGetBoolean - Fix comment
Hi.
IMO the comment ("If no parameter given, ...") is a bit misleading:
====
(gdb) list
108 defGetBoolean(DefElem *def)
109 {
110 /*
111 * If no parameter given, assume "true" is meant.
112 */
113 if (def->arg == NULL)
114 return true;
115
116 /*
117 * Allow 0, 1, "true", "false", "on", "off"
(gdb) p *def
$9 = {type = T_DefElem, defnamespace = 0x0, defname = 0x1c177a8
"copy_data", arg = 0x0,
defaction = DEFELEM_UNSPEC, location = 93}
(gdb)
====
Really this code is for the case when there *was* a parameter given
(e.g. "copy_data" in my example above) but when there is no parameter
*value* given.
Suggested comment fix:
BEFORE
If no parameter given, assume "true" is meant.
AFTER
If no parameter value given, assume "true" is meant.
~~
Although it seems a trivial point, the motivation is that the above
code has been adapted (cut/paste) by multiple other WIP patches [1]/messages/by-id/CALDaNm0gwjY_4HFxvvty01BOT01q_fJLKQ3pWP9=9orqubhjcQ@mail.gmail.com +static char +defGetStreamingMode(DefElem *def)[2]/messages/by-id/CAHut+Ps+4iLzJGkPFEatv=+aa6NUB38-WT050RFKeJqhdcLaGA@mail.gmail.com +static CopyData +DefGetCopyData(DefElem *def)
that I'm aware of, and so this original (misleading) comment is now
spreading to other places.
PSA patch to tweak both the original comment and another place it
already spread to.
------
[1] https://www.postgresql.org/message-id/flat/CALDaNm0gwjY_4HFxvvty01BOT01q_fJLKQ3pWP9%3D9orqubhjcQ%40mail.gmail.com
+static char
+defGetStreamingMode(DefElem *def)[2] https://www.postgresql.org/message-id/flat/CAHut%2BPs%2B4iLzJGkPFEatv%3D%2Baa6NUB38-WT050RFKeJqhdcLaGA%40mail.gmail.com#6d43277cbb074071b8e9602ff8be7e41
+static CopyData
+DefGetCopyData(DefElem *def)Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v1-0001-Fix-comment-in-defGetBoolean.patchapplication/octet-stream; name=v1-0001-Fix-comment-in-defGetBoolean.patchDownload
From 34f2272a49fd04bf616870d5591832a3d48f6cfa Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Wed, 6 Jul 2022 12:16:25 +1000
Subject: [PATCH v1] Fix comment in defGetBoolean
---
src/backend/commands/copy.c | 2 +-
src/backend/commands/define.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e2870e3..3ac7318 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -321,7 +321,7 @@ static CopyHeaderChoice
defGetCopyHeaderChoice(DefElem *def, bool is_from)
{
/*
- * If no parameter given, assume "true" is meant.
+ * If no parameter value given, assume "true" is meant.
*/
if (def->arg == NULL)
return COPY_HEADER_TRUE;
diff --git a/src/backend/commands/define.c b/src/backend/commands/define.c
index 0755ab1..86b8907 100644
--- a/src/backend/commands/define.c
+++ b/src/backend/commands/define.c
@@ -108,7 +108,7 @@ bool
defGetBoolean(DefElem *def)
{
/*
- * If no parameter given, assume "true" is meant.
+ * If no parameter value given, assume "true" is meant.
*/
if (def->arg == NULL)
return true;
--
1.8.3.1
On Thu, Jul 07, 2022 at 09:53:01AM +1000, Peter Smith wrote:
Really this code is for the case when there *was* a parameter given
(e.g. "copy_data" in my example above) but when there is no parameter
*value* given.Suggested comment fix:
BEFORE
If no parameter given, assume "true" is meant.
AFTER
If no parameter value given, assume "true" is meant.
Still, I think that your adjustment is right, as the check is, indeed,
on the DefElem's value*. Or you could just say "If no value given".
--
Michael
On Thu, Jul 07, 2022 at 09:54:24AM +0900, Michael Paquier wrote:
Still, I think that your adjustment is right, as the check is, indeed,
on the DefElem's value*. Or you could just say "If no value given".
Peter, I have applied something using your original wording. This is
a minor issue, but I did not see my suggestion as being better than
yours.
--
Michael
On Mon, Jul 11, 2022 at 12:09 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Jul 07, 2022 at 09:54:24AM +0900, Michael Paquier wrote:
Still, I think that your adjustment is right, as the check is, indeed,
on the DefElem's value*. Or you could just say "If no value given".Peter, I have applied something using your original wording. This is
a minor issue, but I did not see my suggestion as being better than
yours.
Thanks for pushing it.
------
Kind Regards,
Peter Smith.
Fujitsu Australia