Thread: Typo in pg_db_role_setting.h

Typo in pg_db_role_setting.h

From
Japin Li
Date:
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_settingusing 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

From
Richard Guo
Date:

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

From
John Naylor
Date:

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

From
Tom Lane
Date:
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

From
Japin Li
Date:
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

From
Japin Li
Date:
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.


Attachment

Re: Typo in pg_db_role_setting.h

From
Richard Guo
Date:

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

From
John Naylor
Date:

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

From
Japin Li
Date:
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

From
John Naylor
Date:

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

From
Richard Guo
Date:

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

From
Daniel Gustafsson
Date:
> 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

From
Japin Li
Date:
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.