Update EnableTimeoutParams timeout type comment

Started by Xuneng Zhou15 days ago6 messageshackers
Jump to latest
#1Xuneng Zhou
xunengzhou@gmail.com

Hi Hackers,

While reading the timeout code, I noticed that the comment for
EnableTimeoutParams timeout type is kinda stale. Currently, we support
three types of timeout TMPARAM_AFTER/AT/EVERY, but the comment only
mentions the first two. It may not cause confusion, since the two
structs are adjacent, but it might still be worth making this clearer.
Attached a patch to update it.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

Attachments:

v1-0001-Update-EnableTimeoutParams-timeout-type-comment.patchapplication/octet-stream; name=v1-0001-Update-EnableTimeoutParams-timeout-type-comment.patchDownload+1-2
#2Xuneng Zhou
xunengzhou@gmail.com
In reply to: Xuneng Zhou (#1)
Re: Update EnableTimeoutParams timeout type comment

On Wed, Jul 1, 2026 at 9:43 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:

Hi Hackers,

While reading the timeout code, I noticed that the comment for
EnableTimeoutParams timeout type is kinda stale. Currently, we support
three types of timeout TMPARAM_AFTER/AT/EVERY, but the comment only
mentions the first two. It may not cause confusion, since the two
structs are adjacent, but it might still be worth making this clearer.
Attached a patch to update it.

Opus, it should be "the struct and the enum", not two structs.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

#3John Naylor
john.naylor@enterprisedb.com
In reply to: Xuneng Zhou (#1)
Re: Update EnableTimeoutParams timeout type comment

On Wed, Jul 1, 2026 at 8:44 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:

While reading the timeout code, I noticed that the comment for
EnableTimeoutParams timeout type is kinda stale. Currently, we support
three types of timeout TMPARAM_AFTER/AT/EVERY, but the comment only
mentions the first two. It may not cause confusion, since the two
structs are adjacent, but it might still be worth making this clearer.
Attached a patch to update it.

It seems pointless to have a comment whose entire content is to list
every current enum member of that type. I'd just delete the comment,
since it's likely to just going to get outdated again. If any other
objects of type enum have this, they should get the same treatment.

--
John Naylor
Amazon Web Services

#4Xuneng Zhou
xunengzhou@gmail.com
In reply to: John Naylor (#3)
Re: Update EnableTimeoutParams timeout type comment

Hi John,

On Wed, Jul 1, 2026 at 12:01 PM John Naylor <johncnaylorls@gmail.com> wrote:

On Wed, Jul 1, 2026 at 8:44 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:

While reading the timeout code, I noticed that the comment for
EnableTimeoutParams timeout type is kinda stale. Currently, we support
three types of timeout TMPARAM_AFTER/AT/EVERY, but the comment only
mentions the first two. It may not cause confusion, since the two
structs are adjacent, but it might still be worth making this clearer.
Attached a patch to update it.

It seems pointless to have a comment whose entire content is to list
every current enum member of that type. I'd just delete the comment,
since it's likely to just going to get outdated again. If any other
objects of type enum have this, they should get the same treatment.

Agreed. The comment just duplicates nearby code and can easily go
stale again. Removing it also makes sense to me. Here's the patch with
the treatment.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

Attachments:

v2-0001-Remove-stale-EnableTimeoutParams-type-comment.patchapplication/octet-stream; name=v2-0001-Remove-stale-EnableTimeoutParams-type-comment.patchDownload+1-2
#5John Naylor
john.naylor@enterprisedb.com
In reply to: Xuneng Zhou (#4)
Re: Update EnableTimeoutParams timeout type comment

On Wed, Jul 1, 2026 at 11:41 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:

On Wed, Jul 1, 2026 at 12:01 PM John Naylor <johncnaylorls@gmail.com> wrote:

It seems pointless to have a comment whose entire content is to list
every current enum member of that type. I'd just delete the comment,
since it's likely to just going to get outdated again. If any other
objects of type enum have this, they should get the same treatment.

Agreed. The comment just duplicates nearby code and can easily go
stale again. Removing it also makes sense to me. Here's the patch with
the treatment.

My last sentence was meant to imply that I'd rather find and remove
all such comments, rather than fix only one and leave the rest for
someone else to stumble across. I had Claude whip up a throwaway
script to find candidate useless comments, and didn't see any others
like it, only those documenting a value that can have only a subset of
the enum values.

I've pushed your patch.

--
John Naylor
Amazon Web Services

#6Xuneng Zhou
xunengzhou@gmail.com
In reply to: John Naylor (#5)
Re: Update EnableTimeoutParams timeout type comment

On Thu, Jul 2, 2026 at 2:23 PM John Naylor <johncnaylorls@gmail.com> wrote:

On Wed, Jul 1, 2026 at 11:41 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:

On Wed, Jul 1, 2026 at 12:01 PM John Naylor <johncnaylorls@gmail.com> wrote:

It seems pointless to have a comment whose entire content is to list
every current enum member of that type. I'd just delete the comment,
since it's likely to just going to get outdated again. If any other
objects of type enum have this, they should get the same treatment.

Agreed. The comment just duplicates nearby code and can easily go
stale again. Removing it also makes sense to me. Here's the patch with
the treatment.

My last sentence was meant to imply that I'd rather find and remove
all such comments, rather than fix only one and leave the rest for
someone else to stumble across. I had Claude whip up a throwaway
script to find candidate useless comments, and didn't see any others
like it, only those documenting a value that can have only a subset of
the enum values.

Sorry, I didn't get that. Thanks for investigating this.

I've pushed your patch.

Thanks.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.