Обсуждение: Header / Trailer Comment Typos for M4 macros

Hi hackers,

While poking at the build system I stumbled upon some trivial trailer
comment inconsistencies in config/c-compiler.m4. They can be fixed
either way: either changing the macro names or changing the comment. PFA
a patch that keeps the macro names.

In hindsight though, it seems that PGAC_PRINTF_ARCHETYPE was meant to be
PGAC_C_PRINTF_ARCHETYPE, judging by a half-second squint of surrounding,
similar macros. Thoughts?

Also in hindsight: it seems that, as suggested in the trailer typo,
PGAC_PROG_CXX_VAR_OPT (a la the C version PGAC_PROG_CC_VAR_OPT) would be
a good addition if we ever want to add the negative warning flags (for
starter, Wno-unused-command-line-argument for clang++) to CXXFLAGS, but
I assume it wasn't there in the final patch because we didn't use it
(presumably because the patch was minimized?). Thoughts?

Cheers,
Jesse

Jesse Zhang <sbjesse@gmail.com> writes:
> While poking at the build system I stumbled upon some trivial trailer
> comment inconsistencies in config/c-compiler.m4. They can be fixed
> either way: either changing the macro names or changing the comment. PFA
> a patch that keeps the macro names.

Pushed, thanks.

> In hindsight though, it seems that PGAC_PRINTF_ARCHETYPE was meant to be
> PGAC_C_PRINTF_ARCHETYPE, judging by a half-second squint of surrounding,
> similar macros. Thoughts?

Maybe, but I doubt it's worth renaming.

> Also in hindsight: it seems that, as suggested in the trailer typo,
> PGAC_PROG_CXX_VAR_OPT (a la the C version PGAC_PROG_CC_VAR_OPT) would be
> a good addition if we ever want to add the negative warning flags (for
> starter, Wno-unused-command-line-argument for clang++) to CXXFLAGS, but
> I assume it wasn't there in the final patch because we didn't use it
> (presumably because the patch was minimized?). Thoughts?

I'd be inclined not to add it till we have an actual use for it.
Dead code tends to break silently.

            regards, tom lane

On Wed, Apr 22, 2020 at 12:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Jesse Zhang <sbjesse@gmail.com> writes:
> > either way: either changing the macro names or changing the comment. PFA
> > a patch that keeps the macro names.
>
> Pushed, thanks.
>
Thanks!

> > Also in hindsight: it seems that, as suggested in the trailer typo,
> > PGAC_PROG_CXX_VAR_OPT (a la the C version PGAC_PROG_CC_VAR_OPT) would be
> > a good addition if we ever want to add the negative warning flags (for
> > starter, Wno-unused-command-line-argument for clang++) to CXXFLAGS, but
> > I assume it wasn't there in the final patch because we didn't use it
> > (presumably because the patch was minimized?). Thoughts?
>
> I'd be inclined not to add it till we have an actual use for it.
> Dead code tends to break silently.
>
For sure. I feel the same about dead code.

I didn't make my question clear though: I'm curious what motivated the
original addition of -Wno-unused-command-line-argument in commit
73b416b2e412, and how that problem did't quite manifest itself with Clang++.
The commit mentioned pthread flags, but I tried taking out
Wno-unused-command-line-argument from configure.in and it produces no
warnings on my laptop (I know, that's a bad excuse). For context, I'm
running Clang 11 on Ubuntu amd64.

Cheers,
Jesse

Jesse Zhang <sbjesse@gmail.com> writes:
> I didn't make my question clear though: I'm curious what motivated the
> original addition of -Wno-unused-command-line-argument in commit
> 73b416b2e412, and how that problem did't quite manifest itself with Clang++.

We didn't then have the convention of mentioning relevant mailing list
threads in the commit log, but some excavation finds

https://www.postgresql.org/message-id/flat/CALkS6B8Ei3yffHTnUsAovCPmO9kPTpgCArwyod7Ju2eWBm6%3DBA%40mail.gmail.com

So it seems to have been specific to clang circa version 6.0.  Maybe
the clang boys thought better of this behavior more recently?

[ experiments... ]  I see no warnings on current macOS (Apple clang
version 11.0.3) after removing the switch.  So I guess they did fix it.

We're pretty conservative about dropping support for old toolchains,
though, so I doubt that we'd want to remove this configure check.
Especially if we don't know how long ago clang changed it.

            regards, tom lane