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_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.
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
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
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.hb/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.
Attachments:
remove-comments-about-prototypes-for-functions.patchtext/x-patchDownload
diff --git a/src/include/catalog/pg_db_role_setting.h b/src/include/catalog/pg_db_role_setting.h
index 45d478e9e7..997a43e373 100644
--- a/src/include/catalog/pg_db_role_setting.h
+++ b/src/include/catalog/pg_db_role_setting.h
@@ -50,9 +50,6 @@ 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
- */
extern void AlterSetting(Oid databaseid, Oid roleid, VariableSetStmt *setstmt);
extern void DropSetting(Oid databaseid, Oid roleid);
extern void ApplySetting(Snapshot snapshot, Oid databaseid, Oid roleid,
diff --git a/src/include/catalog/pg_enum.h b/src/include/catalog/pg_enum.h
index 9c6deddc6a..497a498e5b 100644
--- a/src/include/catalog/pg_enum.h
+++ b/src/include/catalog/pg_enum.h
@@ -47,9 +47,6 @@ DECLARE_UNIQUE_INDEX_PKEY(pg_enum_oid_index, 3502, EnumOidIndexId, on pg_enum us
DECLARE_UNIQUE_INDEX(pg_enum_typid_label_index, 3503, EnumTypIdLabelIndexId, on pg_enum using btree(enumtypid oid_ops, enumlabel name_ops));
DECLARE_UNIQUE_INDEX(pg_enum_typid_sortorder_index, 3534, EnumTypIdSortOrderIndexId, on pg_enum using btree(enumtypid oid_ops, enumsortorder float4_ops));
-/*
- * prototypes for functions in pg_enum.c
- */
extern void EnumValuesCreate(Oid enumTypeOid, List *vals);
extern void EnumValuesDelete(Oid enumTypeOid);
extern void AddEnumLabel(Oid enumTypeOid, const char *newVal,
diff --git a/src/include/catalog/pg_namespace.h b/src/include/catalog/pg_namespace.h
index ba56e44d61..98eb29d0be 100644
--- a/src/include/catalog/pg_namespace.h
+++ b/src/include/catalog/pg_namespace.h
@@ -56,9 +56,6 @@ DECLARE_TOAST(pg_namespace, 4163, 4164);
DECLARE_UNIQUE_INDEX(pg_namespace_nspname_index, 2684, NamespaceNameIndexId, on pg_namespace using btree(nspname name_ops));
DECLARE_UNIQUE_INDEX_PKEY(pg_namespace_oid_index, 2685, NamespaceOidIndexId, on pg_namespace using btree(oid oid_ops));
-/*
- * prototypes for functions in pg_namespace.c
- */
extern Oid NamespaceCreate(const char *nspName, Oid ownerId, bool isTemp);
#endif /* PG_NAMESPACE_H */
diff --git a/src/include/catalog/pg_range.h b/src/include/catalog/pg_range.h
index faa57e8cea..0918c4317a 100644
--- a/src/include/catalog/pg_range.h
+++ b/src/include/catalog/pg_range.h
@@ -60,10 +60,6 @@ typedef FormData_pg_range *Form_pg_range;
DECLARE_UNIQUE_INDEX_PKEY(pg_range_rngtypid_index, 3542, RangeTypidIndexId, on pg_range using btree(rngtypid oid_ops));
DECLARE_UNIQUE_INDEX(pg_range_rngmultitypid_index, 2228, RangeMultirangeTypidIndexId, on pg_range using btree(rngmultitypid oid_ops));
-/*
- * prototypes for functions in pg_range.c
- */
-
extern void RangeCreate(Oid rangeTypeOid, Oid rangeSubType, Oid rangeCollation,
Oid rangeSubOpclass, RegProcedure rangeCanonical,
RegProcedure rangeSubDiff, Oid multirangeTypeOid);
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 ofobvious
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
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
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 ofobvious
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 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
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 rangetypesI 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
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.