Thread

Commits

Fix comment in pg_db_role_setting.h
- c689baa158bb 16.0 landed

Typo in pg_db_role_setting.h

Japin Li <japinli@hotmail.com> — 2022-08-01T09:18:39Z

Hi, hackers

I think there is a typo in pg_db_role_setting.h, should we fix it?

diff --git a/src/include/catalog/pg_db_role_setting.h b/src/include/catalog/pg_db_role_setting.h
index 45d478e9e7..f92e867df4 100644
--- a/src/include/catalog/pg_db_role_setting.h
+++ b/src/include/catalog/pg_db_role_setting.h
@@ -51,7 +51,7 @@ DECLARE_TOAST_WITH_MACRO(pg_db_role_setting, 2966, 2967, PgDbRoleSettingToastTab
 DECLARE_UNIQUE_INDEX_PKEY(pg_db_role_setting_databaseid_rol_index, 2965, DbRoleSettingDatidRolidIndexId, on pg_db_role_setting using btree(setdatabase oid_ops, setrole oid_ops));

 /*
- * prototypes for functions in pg_db_role_setting.h
+ * prototypes for functions in pg_db_role_setting.c
  */
 extern void AlterSetting(Oid databaseid, Oid roleid, VariableSetStmt *setstmt);
 extern void DropSetting(Oid databaseid, Oid roleid);

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Typo in pg_db_role_setting.h

Richard Guo <guofenglinux@gmail.com> — 2022-08-01T09:28:45Z

On Mon, Aug 1, 2022 at 5:18 PM Japin Li <japinli@hotmail.com> wrote:

> I think there is a typo in pg_db_role_setting.h, should we fix it?


Definitely this is wrong. +1 for the fix.

Thanks
Richard

Re: Typo in pg_db_role_setting.h

John Naylor <john.naylor@enterprisedb.com> — 2022-08-01T12:46:30Z

On Mon, Aug 1, 2022 at 4:18 PM Japin Li <japinli@hotmail.com> wrote:
>
>
> Hi, hackers
>
> I think there is a typo in pg_db_role_setting.h, should we fix it?
>
> diff --git a/src/include/catalog/pg_db_role_setting.h
b/src/include/catalog/pg_db_role_setting.h
> index 45d478e9e7..f92e867df4 100644
>  /*
> - * prototypes for functions in pg_db_role_setting.h
> + * prototypes for functions in pg_db_role_setting.c
>   */

You are correct, but I wonder if it'd be better to just drop the comment
entirely. I checked a couple other random headers with function
declarations and they didn't have such a comment, and it's kind of obvious
what they're for.

--
John Naylor
EDB: http://www.enterprisedb.com

Re: Typo in pg_db_role_setting.h

Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-01T14:16:08Z

John Naylor <john.naylor@enterprisedb.com> writes:
> You are correct, but I wonder if it'd be better to just drop the comment
> entirely. I checked a couple other random headers with function
> declarations and they didn't have such a comment, and it's kind of obvious
> what they're for.

Some places have these, some don't.  It's probably more useful where
a header foo.h is declaring functions that aren't in the obviously
corresponding foo.c file, or live in multiple files.  In this case
I agree it's not adding much.

			regards, tom lane

Re: Typo in pg_db_role_setting.h

Japin Li <japinli@hotmail.com> — 2022-08-01T14:24:24Z

On Mon, 01 Aug 2022 at 20:46, John Naylor <john.naylor@enterprisedb.com> wrote:
> On Mon, Aug 1, 2022 at 4:18 PM Japin Li <japinli@hotmail.com> wrote:
>>
>>
>> Hi, hackers
>>
>> I think there is a typo in pg_db_role_setting.h, should we fix it?
>>
>> diff --git a/src/include/catalog/pg_db_role_setting.h
> b/src/include/catalog/pg_db_role_setting.h
>> index 45d478e9e7..f92e867df4 100644
>>  /*
>> - * prototypes for functions in pg_db_role_setting.h
>> + * prototypes for functions in pg_db_role_setting.c
>>   */
>
> You are correct, but I wonder if it'd be better to just drop the comment
> entirely. I checked a couple other random headers with function
> declarations and they didn't have such a comment, and it's kind of obvious
> what they're for.

Both are fine for me.  I find there are some headers also have such a comment,
like pg_enum, pg_range and pg_namespace.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Typo in pg_db_role_setting.h

Japin Li <japinli@hotmail.com> — 2022-08-01T14:42:22Z

On Mon, 01 Aug 2022 at 22:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> John Naylor <john.naylor@enterprisedb.com> writes:
>> You are correct, but I wonder if it'd be better to just drop the comment
>> entirely. I checked a couple other random headers with function
>> declarations and they didn't have such a comment, and it's kind of obvious
>> what they're for.
>
> Some places have these, some don't.  It's probably more useful where
> a header foo.h is declaring functions that aren't in the obviously
> corresponding foo.c file, or live in multiple files.  In this case
> I agree it's not adding much.
>

Attached patch to remove this comment.  Please take a look.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Typo in pg_db_role_setting.h

Richard Guo <guofenglinux@gmail.com> — 2022-08-02T03:06:01Z

On Mon, Aug 1, 2022 at 10:42 PM Japin Li <japinli@hotmail.com> wrote:

>
> On Mon, 01 Aug 2022 at 22:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > John Naylor <john.naylor@enterprisedb.com> writes:
> >> You are correct, but I wonder if it'd be better to just drop the comment
> >> entirely. I checked a couple other random headers with function
> >> declarations and they didn't have such a comment, and it's kind of
> obvious
> >> what they're for.
> >
> > Some places have these, some don't.  It's probably more useful where
> > a header foo.h is declaring functions that aren't in the obviously
> > corresponding foo.c file, or live in multiple files.  In this case
> > I agree it's not adding much.
> >
>
> Attached patch to remove this comment.  Please take a look.


I'm not sure that we should remove such comments. And a rough search
shows that there are much more places with this kind of comments, such
as below:

nbtxlog transam readfuncs walreceiver buffile bufmgr fd latch pmsignal
procsignal sinvaladt logtape rangetypes

Thanks
Richard

Re: Typo in pg_db_role_setting.h

John Naylor <john.naylor@enterprisedb.com> — 2022-08-02T04:13:25Z

On Tue, Aug 2, 2022 at 10:06 AM Richard Guo <guofenglinux@gmail.com> wrote:
>
> I'm not sure that we should remove such comments. And a rough search
> shows that there are much more places with this kind of comments, such
> as below:
>
> nbtxlog transam readfuncs walreceiver buffile bufmgr fd latch pmsignal
> procsignal sinvaladt logtape rangetypes

I was talking only about catalog/pg_*.c functions, as in Japin Li's latest
patch. You didn't mention whether your examples fall in the category Tom
mentioned upthread, so I'm not sure what your angle is.

--
John Naylor
EDB: http://www.enterprisedb.com

Re: Typo in pg_db_role_setting.h

Japin Li <japinli@hotmail.com> — 2022-08-02T04:34:29Z

On Tue, 02 Aug 2022 at 11:06, Richard Guo <guofenglinux@gmail.com> wrote:
> On Mon, Aug 1, 2022 at 10:42 PM Japin Li <japinli@hotmail.com> wrote:
>
>>
>> On Mon, 01 Aug 2022 at 22:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > John Naylor <john.naylor@enterprisedb.com> writes:
>> >> You are correct, but I wonder if it'd be better to just drop the comment
>> >> entirely. I checked a couple other random headers with function
>> >> declarations and they didn't have such a comment, and it's kind of
>> obvious
>> >> what they're for.
>> >
>> > Some places have these, some don't.  It's probably more useful where
>> > a header foo.h is declaring functions that aren't in the obviously
>> > corresponding foo.c file, or live in multiple files.  In this case
>> > I agree it's not adding much.
>> >
>>
>> Attached patch to remove this comment.  Please take a look.
>
>
> I'm not sure that we should remove such comments. And a rough search
> shows that there are much more places with this kind of comments, such
> as below:
>
> nbtxlog transam readfuncs walreceiver buffile bufmgr fd latch pmsignal
> procsignal sinvaladt logtape rangetypes
>

Thanks for your review! Here, I think we are only talking about catalog headers.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Typo in pg_db_role_setting.h

John Naylor <john.naylor@enterprisedb.com> — 2022-08-02T04:43:19Z

On Mon, Aug 1, 2022 at 9:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> John Naylor <john.naylor@enterprisedb.com> writes:
> > You are correct, but I wonder if it'd be better to just drop the comment
> > entirely. I checked a couple other random headers with function
> > declarations and they didn't have such a comment, and it's kind of
obvious
> > what they're for.
>
> Some places have these, some don't.  It's probably more useful where
> a header foo.h is declaring functions that aren't in the obviously
> corresponding foo.c file, or live in multiple files.  In this case
> I agree it's not adding much.

I somehow forgot that just yesterday I working on a project that will
possibly add a declaration to every catalog header for tuple deforming. In
that case, we will want to keep existing comments and possibly add more. In
the meantime, I'll go just apply the correction.

--
John Naylor
EDB: http://www.enterprisedb.com

Re: Typo in pg_db_role_setting.h

Richard Guo <guofenglinux@gmail.com> — 2022-08-02T07:37:58Z

On Tue, Aug 2, 2022 at 12:13 PM John Naylor <john.naylor@enterprisedb.com>
wrote:

>
> On Tue, Aug 2, 2022 at 10:06 AM Richard Guo <guofenglinux@gmail.com>
> wrote:
> >
> > I'm not sure that we should remove such comments. And a rough search
> > shows that there are much more places with this kind of comments, such
> > as below:
> >
> > nbtxlog transam readfuncs walreceiver buffile bufmgr fd latch pmsignal
> > procsignal sinvaladt logtape rangetypes
>
> I was talking only about catalog/pg_*.c functions, as in Japin Li's latest
> patch. You didn't mention whether your examples fall in the category Tom
> mentioned upthread, so I'm not sure what your angle is.
>

Sorry I forgot to mention that. The examples listed upthread all contain
such comment in foo.h saying 'prototypes for functions in foo.c'. For
instance, in buffile.h, there is comment saying

/*
 * prototypes for functions in buffile.c
 */

So if we remove such comments, should we also do so for those cases?

Thanks
Richard

Re: Typo in pg_db_role_setting.h

Daniel Gustafsson <daniel@yesql.se> — 2022-08-02T07:45:36Z

> On 2 Aug 2022, at 09:37, Richard Guo <guofenglinux@gmail.com> wrote:

> The examples listed upthread all contain such comment in foo.h saying
> 'prototypes for functions in foo.c'.  For instance, in buffile.h, there is
> comment saying

> /*
>  * prototypes for functions in buffile.c
>  */
> 
> So if we remove such comments, should we also do so for those cases?

Comments which state the obvious are seldom helpful, I would prefer to remove
such comments and only explicitly call out the .c file in a comment when it's a
different basename from the header.

--
Daniel Gustafsson		https://vmware.com/

Re: Typo in pg_db_role_setting.h

Japin Li <japinli@hotmail.com> — 2022-08-02T07:58:40Z

On Tue, 02 Aug 2022 at 15:45, Daniel Gustafsson <daniel@yesql.se> wrote:
>> On 2 Aug 2022, at 09:37, Richard Guo <guofenglinux@gmail.com> wrote:
>
>> The examples listed upthread all contain such comment in foo.h saying
>> 'prototypes for functions in foo.c'.  For instance, in buffile.h, there is
>> comment saying
>
>> /*
>>  * prototypes for functions in buffile.c
>>  */
>> 
>> So if we remove such comments, should we also do so for those cases?
>
> Comments which state the obvious are seldom helpful, I would prefer to remove
> such comments and only explicitly call out the .c file in a comment when it's a
> different basename from the header.

+1

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.