Thread: Typo in pg_db_role_setting.h

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.

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

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.

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.

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.

> 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/

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.