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
Attachments:
0001-Fix-header-trailer-typos-for-m4-macros.patchapplication/x-patch; name=0001-Fix-header-trailer-typos-for-m4-macros.patchDownload
From 73c15bfdea388347455810c3eb98dbff235d3d08 Mon Sep 17 00:00:00 2001
From: Jesse Zhang <sbjesse@gmail.com>
Date: Wed, 22 Apr 2020 06:51:05 -0700
Subject: [PATCH] Fix header / trailer typos for m4 macros
These seem like legit names in earlier iterations of respective patches
(commit b779168ffe33 "Detect PG_PRINTF_ATTRIBUTE automatically." and
commit 6869b4f25847 "Add C++ support to configure.") but the macro had
been renamed out of sync with the header / trailer comment in the final
committed patch.
---
config/c-compiler.m4 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
index 501b74b3a19..c48cba9cf9a 100644
--- a/config/c-compiler.m4
+++ b/config/c-compiler.m4
@@ -2,7 +2,7 @@
# config/c-compiler.m4
-# PGAC_C_PRINTF_ARCHETYPE
+# PGAC_PRINTF_ARCHETYPE
# -----------------------
# Select the format archetype to be used by gcc to check printf-type functions.
# We prefer "gnu_printf", as that most closely matches the features supported
@@ -466,7 +466,7 @@ undefine([Ac_cachevar])dnl
# command-line option. If it does, add the string to CXXFLAGS.
AC_DEFUN([PGAC_PROG_CXX_CFLAGS_OPT],
[PGAC_PROG_VARCXX_VARFLAGS_OPT(CXX, CXXFLAGS, $1)
-])# PGAC_PROG_CXX_VAR_OPT
+])# PGAC_PROG_CXX_CFLAGS_OPT
--
2.26.2
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
/messages/by-id/CALkS6B8Ei3yffHTnUsAovCPmO9kPTpgCArwyod7Ju2eWBm6=BA@mail.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