Thread: remove useless comments

The comments considering checking share/ directory was there for
almost 13 years, yet nobody ever trying to add the checking, and
there seems never any trouble for not checking it, then I think
we should remove those comments.

diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index 81cb585891..ecdc59ce5e 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1581,11 +1581,6 @@ getInstallationPaths(const char *argv0)
                                 errhint("This may indicate an
incomplete PostgreSQL installation, or that the file \"%s\" has been
moved away from its proper location.",
                                                 my_exec_path)));
        FreeDir(pdir);
-
-       /*
-        * XXX is it worth similarly checking the share/ directory?  If the lib/
-        * directory is there, then share/ probably is too.
-        */
 }

-- 
Regards
Junwang Zhao

Junwang Zhao <zhjwpku@gmail.com> writes:
> The comments considering checking share/ directory was there for
> almost 13 years, yet nobody ever trying to add the checking, and
> there seems never any trouble for not checking it, then I think
> we should remove those comments.

I think that comment is valuable.  It shows that checking the
sibling directories was considered and didn't seem worthwhile.
Perhaps it should be rephrased in a more positive way (without XXX),
but merely deleting it is a net negative because future hackers
would have to reconstruct that reasoning.

BTW, we're working in a 30+-year-old code base, so the mere fact
that a comment has been there a long time does not make it bad.

            regards, tom lane

Fair enough, the rephased version of the comments is in the attachment,
please take a look.

--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1583,8 +1583,8 @@ getInstallationPaths(const char *argv0)
  FreeDir(pdir);

  /*
- * XXX is it worth similarly checking the share/ directory?  If the lib/
- * directory is there, then share/ probably is too.
+ * It's not worth checking the share/ directory.  If the lib/ directory
+ * is there, then share/ probably is too.
  */
 }

On Tue, Aug 9, 2022 at 11:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Junwang Zhao <zhjwpku@gmail.com> writes:
> > The comments considering checking share/ directory was there for
> > almost 13 years, yet nobody ever trying to add the checking, and
> > there seems never any trouble for not checking it, then I think
> > we should remove those comments.
>
> I think that comment is valuable.  It shows that checking the
> sibling directories was considered and didn't seem worthwhile.
> Perhaps it should be rephrased in a more positive way (without XXX),
> but merely deleting it is a net negative because future hackers
> would have to reconstruct that reasoning.
>
> BTW, we're working in a 30+-year-old code base, so the mere fact
> that a comment has been there a long time does not make it bad.
>
>                         regards, tom lane



-- 
Regards
Junwang Zhao

On Wed, Aug 10, 2022 at 12:24:02AM +0800, Junwang Zhao wrote:
> Fair enough, the rephased version of the comments is in the attachment,
> please take a look.
> 
> --- a/src/backend/postmaster/postmaster.c
> +++ b/src/backend/postmaster/postmaster.c
> @@ -1583,8 +1583,8 @@ getInstallationPaths(const char *argv0)
>   FreeDir(pdir);
> 
>   /*
> - * XXX is it worth similarly checking the share/ directory?  If the lib/
> - * directory is there, then share/ probably is too.
> + * It's not worth checking the share/ directory.  If the lib/ directory
> + * is there, then share/ probably is too.
>   */
>  }

Patch applied to master.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.